From: Yan-Hsuan Chuang <[email protected]>
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 <[email protected]>
---
drivers/net/wireless/realtek/rtw88/main.c | 7 +++++--
drivers/net/wireless/realtek/rtw88/ps.h | 2 --
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 7c1b89c4fb6c..bff8a0b129d9 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -16,16 +16,19 @@
#include "debug.h"
#include "bf.h"
+unsigned int rtw_lps_threshold = 2;
unsigned int rtw_fw_lps_deep_mode;
EXPORT_SYMBOL(rtw_fw_lps_deep_mode);
bool rtw_bf_support = true;
unsigned int rtw_debug_mask;
EXPORT_SYMBOL(rtw_debug_mask);
+module_param_named(lps_threshold, rtw_lps_threshold, uint, 0644);
module_param_named(lps_deep_mode, rtw_fw_lps_deep_mode, uint, 0644);
module_param_named(support_bf, rtw_bf_support, bool, 0644);
module_param_named(debug_mask, rtw_debug_mask, uint, 0644);
+MODULE_PARM_DESC(lps_threshold, "Threshold of number of packets in every 2 seconds");
MODULE_PARM_DESC(lps_deep_mode, "Deeper PS mode. If 0, deep PS is disabled");
MODULE_PARM_DESC(support_bf, "Set Y to enable beamformee support");
MODULE_PARM_DESC(debug_mask, "Debugging mask");
@@ -199,8 +202,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
if (busy_traffic != test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags))
rtw_coex_wl_status_change_notify(rtwdev);
- if (stats->tx_cnt > RTW_LPS_THRESHOLD ||
- stats->rx_cnt > RTW_LPS_THRESHOLD)
+ if (stats->tx_cnt > rtw_lps_threshold ||
+ stats->rx_cnt > rtw_lps_threshold)
ps_active = true;
else
ps_active = false;
diff --git a/drivers/net/wireless/realtek/rtw88/ps.h b/drivers/net/wireless/realtek/rtw88/ps.h
index 25925eedbad4..fe43f8d96d04 100644
--- a/drivers/net/wireless/realtek/rtw88/ps.h
+++ b/drivers/net/wireless/realtek/rtw88/ps.h
@@ -5,8 +5,6 @@
#ifndef __RTW_PS_H_
#define __RTW_PS_H_
-#define RTW_LPS_THRESHOLD 2
-
#define POWER_MODE_ACK BIT(6)
#define POWER_MODE_PG BIT(4)
#define POWER_MODE_LCLK BIT(0)
--
2.17.1
On Fri, Oct 25, 2019 at 5:33 PM <[email protected]> wrote:
>
> From: Yan-Hsuan Chuang <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/main.c | 7 +++++--
> drivers/net/wireless/realtek/rtw88/ps.h | 2 --
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index 7c1b89c4fb6c..bff8a0b129d9 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -199,8 +202,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
> if (busy_traffic != test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags))
> rtw_coex_wl_status_change_notify(rtwdev);
>
> - if (stats->tx_cnt > RTW_LPS_THRESHOLD ||
> - stats->rx_cnt > RTW_LPS_THRESHOLD)
> + if (stats->tx_cnt > rtw_lps_threshold ||
> + stats->rx_cnt > rtw_lps_threshold)
> ps_active = true;
> else
> ps_active = false;
The naming of 'ps_active' is a bit confusing. Per the commit message,
it will leave LPS
it tx/rx count > threshold. But I'll be misled by the name ps_active.
Does it mean the
current condition is PS active and ready to power sleep? I'd like to
rename it to old-fashioned
'lps_enter' to represent the action that would be taken. It would be
easier for me to understand.
Chris
> --
> 2.17.1
>
> On Fri, Oct 25, 2019 at 5:33 PM <[email protected]> wrote:
> >
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > drivers/net/wireless/realtek/rtw88/main.c | 7 +++++--
> > drivers/net/wireless/realtek/rtw88/ps.h | 2 --
> > 2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> b/drivers/net/wireless/realtek/rtw88/main.c
> > index 7c1b89c4fb6c..bff8a0b129d9 100644
> > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
>
> > @@ -199,8 +202,8 @@ static void rtw_watch_dog_work(struct
> work_struct *work)
> > if (busy_traffic != test_bit(RTW_FLAG_BUSY_TRAFFIC,
> rtwdev->flags))
> > rtw_coex_wl_status_change_notify(rtwdev);
> >
> > - if (stats->tx_cnt > RTW_LPS_THRESHOLD ||
> > - stats->rx_cnt > RTW_LPS_THRESHOLD)
> > + if (stats->tx_cnt > rtw_lps_threshold ||
> > + stats->rx_cnt > rtw_lps_threshold)
> > ps_active = true;
> > else
> > ps_active = false;
>
> The naming of 'ps_active' is a bit confusing. Per the commit message,
> it will leave LPS
> it tx/rx count > threshold. But I'll be misled by the name ps_active.
> Does it mean the
> current condition is PS active and ready to power sleep? I'd like to
> rename it to old-fashioned
> 'lps_enter' to represent the action that would be taken. It would be
> easier for me to understand.
>
> Chris
>
I think according to the context, ps_active is good for me.
But I can still send a separate patch to rename it.
Or you can :)
Yan-Hsuan
On Mon, Oct 28, 2019 at 11:13 AM Tony Chuang <[email protected]> wrote:
>
> > On Fri, Oct 25, 2019 at 5:33 PM <[email protected]> wrote:
> > >
> > > From: Yan-Hsuan Chuang <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
Reviewed-by: Chris Chiu <[email protected]>
> > > drivers/net/wireless/realtek/rtw88/main.c | 7 +++++--
> > > drivers/net/wireless/realtek/rtw88/ps.h | 2 --
> > > 2 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> > b/drivers/net/wireless/realtek/rtw88/main.c
> > > index 7c1b89c4fb6c..bff8a0b129d9 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> >
> > > @@ -199,8 +202,8 @@ static void rtw_watch_dog_work(struct
> > work_struct *work)
> > > if (busy_traffic != test_bit(RTW_FLAG_BUSY_TRAFFIC,
> > rtwdev->flags))
> > > rtw_coex_wl_status_change_notify(rtwdev);
> > >
> > > - if (stats->tx_cnt > RTW_LPS_THRESHOLD ||
> > > - stats->rx_cnt > RTW_LPS_THRESHOLD)
> > > + if (stats->tx_cnt > rtw_lps_threshold ||
> > > + stats->rx_cnt > rtw_lps_threshold)
> > > ps_active = true;
> > > else
> > > ps_active = false;
> >
> > The naming of 'ps_active' is a bit confusing. Per the commit message,
> > it will leave LPS
> > it tx/rx count > threshold. But I'll be misled by the name ps_active.
> > Does it mean the
> > current condition is PS active and ready to power sleep? I'd like to
> > rename it to old-fashioned
> > 'lps_enter' to represent the action that would be taken. It would be
> > easier for me to understand.
> >
> > Chris
> >
>
> I think according to the context, ps_active is good for me.
> But I can still send a separate patch to rename it.
> Or you can :)
>
> Yan-Hsuan
OK. Then I have no problem with this patch.
Chris
<[email protected]> wrote:
> From: Yan-Hsuan Chuang <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Chris Chiu <[email protected]>
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.
--
https://patchwork.kernel.org/patch/11211881/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
From: Kalle Valo
> <[email protected]> wrote:
>
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Chris Chiu <[email protected]>
>
> 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.
Yan-Hsuan
On Thu, Oct 31, 2019 at 1:17 AM Tony Chuang <[email protected]> wrote:
> 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.
FWIW, I think choosing a more reasonable default is definitely a good
start, as long as this choice doesn't have huge downsides.
@Kalle: FYI, this (set to 50) is exactly the change that Tony is
recommending to me for my distro, and I have the same qualms about
supporting a growing number of module parameter tweaks like this. So,
thanks for pushing back :)
Brian
>
> On Thu, Oct 31, 2019 at 1:17 AM Tony Chuang <[email protected]>
> wrote:
> > 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.
>
> FWIW, I think choosing a more reasonable default is definitely a good
> start, as long as this choice doesn't have huge downsides.
>
> @Kalle: FYI, this (set to 50) is exactly the change that Tony is
> recommending to me for my distro, and I have the same qualms about
> supporting a growing number of module parameter tweaks like this. So,
> thanks for pushing back :)
>
> Brian
>
I was afraid of you thinking that setting this to 50 is a strange thing.
But it seems like you'd prefer to change the default value instead of adding a
module parameter to control it. I think we can drop this one and I will send
a patch to change the default value to 50.
Yan-Hsuan
Tony Chuang <[email protected]> writes:
> From: Kalle Valo
>> <[email protected]> wrote:
>>
>> > From: Yan-Hsuan Chuang <[email protected]>
>> >
>> > 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 <[email protected]>
>> > Reviewed-by: Chris Chiu <[email protected]>
>>
>> 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/[email protected]/
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
Tony Chuang <[email protected]> writes:
>> On Thu, Oct 31, 2019 at 1:17 AM Tony Chuang <[email protected]>
>> wrote:
>> > 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.
>>
>> FWIW, I think choosing a more reasonable default is definitely a good
>> start, as long as this choice doesn't have huge downsides.
>>
>> @Kalle: FYI, this (set to 50) is exactly the change that Tony is
>> recommending to me for my distro, and I have the same qualms about
>> supporting a growing number of module parameter tweaks like this. So,
>> thanks for pushing back :)
>>
>> Brian
>>
>
> I was afraid of you thinking that setting this to 50 is a strange thing.
> But it seems like you'd prefer to change the default value instead of adding a
> module parameter to control it. I think we can drop this one and I will send
> a patch to change the default value to 50.
Yeah, as the first step changing to 50 sounds good to me. Later, if
needed, we can extend it to make it configurable from user space, either
via the QoS framework or something else.
--
Kalle Valo