From: Mike Silva <mike.silva@mikesilva.com>
To: James Prestwood <prestwoj@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>, iwd@lists.linux.dev
Subject: Re: powersave support
Date: Sat, 10 Jun 2023 20:26:35 -0700 [thread overview]
Message-ID: <D71AF564-66F5-4542-91D2-E226FE078D94@mikesilva.com> (raw)
In-Reply-To: <772f5654-5232-6aa1-5ef1-601585d408f0@gmail.com>
Hello,
> On Jun 9, 2023, at 3:07 PM, James Prestwood <prestwoj@gmail.com> wrote:
>
> Hi,
>
> On 5/9/23 9:09 AM, Marcel Holtmann wrote:
>> Hi James,
>>>>>> Does IWD support disabling/enabling WiFi powersave/power management?
>>>>>
>>>>> No, currently it doesn't.
>>>>>
>>>>>> Perhaps, I’m failing in finding it in the documentation. It’s possible to do this as needed with ifconfig, iwconfig, or NetworkManager, or to permanently configure them to enable or disable it at boot..
>>>>>> If IWD doesn’t support this, it would be nice enhancement if IWD could when managing the adapter/connection, as some drivers do a poor job of handling powersave being on.
>>>>>
>>>>> Yes, I've recently seen instances of drivers handling this poorly. Are you envisioning something like these other tools, where power save is just toggled on/off?
>>>>>
>>>>> I think this could be done, and actually we do this manually every boot using iwconfig in the software stack I work with. The tricky part is PS is done per-interface and IWD doesn't have any concept of per-interface configurations.
>>>> if anybody uses iwconfig, then the problem is already there. iwconfig
>>>> is an WEXT tool and deprecated. The driver is broken.
>>>
>>> It doesn't have much to do with the tool, its just an NL80211 API to toggle power save. But yes, the driver is broken if it can't handle this properly... but this is the world we live in.
>>>
>>> The reason I'm interested though is because there is no nice way to disable power save "automatically" unless you use NetworkManager. Apart from hooking in a script to disable it with iw/iwconfig (which we currently do). An option in IWD would be convenient.
>>>
>>>>> Denis might have preferences on this? but my initial thoughts would be something like:
>>>>>
>>>>> [General.wlan0]
>>>>> PowerSave={on,off}
>>>> You can't tie anything to interface names. Then can change and are
>>>> basically random names. The most you can do is tie it to the phy
>>>> and its mode of operation.
>>>
>>> True, it would have to be per-phy. But this too has the same problem if its a hotplug device...
>>>
>>> We do have this concept of driver_flags already, for interface removal, but I'm not sure if power save is something that EVERYONE will want to disable for a given driver. Maybe it works for some use cases but not others.
>> we already have this:
>> enum driver_flag {
>> DEFAULT_IF = 0x1,
>> FORCE_PAE = 0x2,
>> };
>> struct driver_info {
>> const char *prefix;
>> unsigned int flags;
>> };
>> /*
>> * The out-of-tree rtl88x2bu crashes the kernel hard if default interface is
>> * destroyed. It seems many other drivers are built from the same source code
>> * so we set the DEFAULT_IF flag for all of them. Unfortunately there are
>> * in-tree drivers that also match these names and may be fine.
>> */
>> static const struct driver_info driver_infos[] = {
>> { "rtl81*", DEFAULT_IF },
>> { "rtl87*", DEFAULT_IF },
>> { "rtl88*", DEFAULT_IF },
>> { "rtw_*", DEFAULT_IF },
>> { "brcmfmac", DEFAULT_IF },
>> { "bcmsdh_sdmmc", DEFAULT_IF },
>> };
>> It is hardcoded and obviously awful as that, but you could add a flag
>> for power save mode. However I think this needs to move into a configuration
>> file somewhere that can match and flag drivers and/or hardware.
>
> After looking into power save more I decided to revisit this sooner than later. I initially thought you could get a persistent PS setting by a script that runs at boot but the way IWD works deleting the default interface actually blows away any PS setting because its tied to the interface. So you would actually have to wait for IWD to start and create interfaces, then set PS off, which is problematic if you don't have a daemon monitoring IWD's state. It would be convenient for IWD to set it once it creates the interface.
I’m annoyed that I didn’t articulate that this was a further problem I was hoping IWD controlling PS would address. Thanks.
>
> So now I'm trying to come up with a clean way of making driver quirks like this configurable via main.conf. I think we should keep this static list for known offenders and tack it onto the end of a dynamic list that gets generated at runtime based on any driver quirks in main.conf
>
> The configuration format is where it gets kinda nasty. I'm thinking of having a "DriverQuirks" group where each key corresponds to a driver flag, and each value is a comma-separated list of glob matches:
>
> [DriverQuirks]
> DefaultInterface=rtl81*,rtl87*,rtl88*,brcmfmac
> PowerSaveOff=ath10k,iwlwifi,rl*
> ForcePae=drv1,drv2,etc
You’ll find that among the Mediatek wifi chipsets, the ones based on the design they’ve purchased from Realtek also carry forward the buggy powersave. I believe it was one of the rt chipsets already in your list. The Mediateks are the mt76, and its derived chipesets like the mt7921. You can find a list of them, here:
https://wireless.wiki.kernel.org/en/users/drivers/mediatek
I’m not sure how complete that list is, but it at least captures a bunch of those which are derived from rt and the rt drivers.
A passing glance at their mailing list looks like they may have been doing some work to address this on some of these chipsets, but in my experience it’s not fixed. (And, I’m running their latest firmware from their git repository.)
>
> One downside here is you may have duplicates between each quirk. Using driver names as the keys would remove duplication, but loses the ability to glob match since '*' is not a valid key character (maybe this is fine though).
>
> I'm open to suggestions here. Trying to find a happy medium between readability and parse-ability.
>
> Thanks,
> James
>
>> Regards
>> Marcel
next prev parent reply other threads:[~2023-06-11 3:26 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-09 2:50 powersave support Mike Silva
2023-05-09 14:17 ` James Prestwood
2023-05-09 14:55 ` Marcel Holtmann
2023-05-09 15:11 ` James Prestwood
2023-05-09 16:09 ` Marcel Holtmann
2023-05-09 16:12 ` James Prestwood
2023-05-09 16:52 ` Mike Silva
2023-06-09 22:07 ` James Prestwood
2023-06-11 3:26 ` Mike Silva [this message]
2023-05-09 16:46 ` Mike Silva
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D71AF564-66F5-4542-91D2-E226FE078D94@mikesilva.com \
--to=mike.silva@mikesilva.com \
--cc=iwd@lists.linux.dev \
--cc=marcel@holtmann.org \
--cc=prestwoj@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).