Re: [PATCH 3/6] rtw88: use a module parameter to control LPS enter

[Kalle Valo <kvalo@codeaurora.org>]

Tony Chuang <yhchuang@realtek.com> writes:

> From: Kalle Valo
>> <yhchuang@realtek.com> wrote:
>> 
>> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> >
>> > If the number of packets is less than the LPS threshold, driver
>> > can then enter LPS mode.
>> > And driver used to take RTW_LPS_THRESHOLD as the threshold. As
>> > the macro can not be changed after compiled, use a parameter
>> > instead.
>> >
>> > The larger of the threshold, the more traffic required to leave
>> > power save mode, responsive time could be longer, but also the
>> > power consumption could be lower.
>> >
>> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> > Reviewed-by: Chris Chiu <chiu@endlessm.com>
>> 
>> I don't think a module parameter should be used to control power save
>> level, instead there should be a generic interface for that. Also the commit
>> log does not give any explanation why this needs to be a module parameter.
>> 
>> Tony, there's a high barrier for adding new module parameters. It's a
>> common
>> phrase for me to say "module parameters are not windows .ini files". And to
>> make it
>> easier for everyone always submit controversial patches separately, do not
>> hide
>> within a bigger patchset.
>> 
>
> Alright, I was thinking module parameters as a convenient tool for driver to
> control the behavior for debugging or out-of-band adjusting. But it seems like
> you treat it more carefully.
>
> Actually this is just going to allow us to set different default values for different
> use cases. So is there a better way to control it. Or I should just change the
> value to a better one. By our experience, set this to 50 is a more reasonable
> value, such that some web surfing or background traffic wouldn't make the
> driver to leave PS mode.

I recall having a similar discussion something like 10 years ago. (Yes,
I have been here for way too long). I think at the time recommendation
was to use latency value from the QoS framework to make it possible for
user space to change wireless power save aggressiveness. But I don't
know if anyone really used that.

I was feeling nostalgic and decided to find some pointers:

https://lore.kernel.org/linux-wireless/1271850458-32437-2-git-send-email-juuso.oikarinen@nokia.com/

And it seems the patch was even applied:

195e294d21e8 mac80211: Determine dynamic PS timeout based on ps-qos network latency

This is for mac80211 dynamic ps feature, but I imagine we could somehow
extend it to driver settings like the LPS threshold here. Something like
this would be much more acceptable than having custom module parameters
for each driver.

-- 
Kalle Valo