Tony Chuang <yhchuang@xxxxxxxxxxx> writes: > From: Kalle Valo >> <yhchuang@xxxxxxxxxxx> wrote: >> >> > From: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx> >> > >> > 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@xxxxxxxxxxx> >> > Reviewed-by: Chris Chiu <chiu@xxxxxxxxxxxx> >> >> 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@xxxxxxxxx/ 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
