| Message ID | 20191025093345.22643-4-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: minor driver updates | expand |
On Fri, Oct 25, 2019 at 5:33 PM <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> > --- > 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 <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> > > --- > > 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 <yhchuang@realtek.com> wrote: > > > On Fri, Oct 25, 2019 at 5:33 PM <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> > > > 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
<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.
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. Yan-Hsuan
On Thu, Oct 31, 2019 at 1:17 AM Tony Chuang <yhchuang@realtek.com> 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 <yhchuang@realtek.com> > 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 <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.
Tony Chuang <yhchuang@realtek.com> writes: >> On Thu, Oct 31, 2019 at 1:17 AM Tony Chuang <yhchuang@realtek.com> >> 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.
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)