Re: [PATCH v2 03/33] staging: wfx: ignore PS when STA/AP share same channel
On Mon, Sep 13, 2021 at 12:36:25PM +0200, Jérôme Pouiller wrote: > On Monday 13 September 2021 11:33:28 CEST Dan Carpenter wrote: > > On Mon, Sep 13, 2021 at 10:30:15AM +0200, Jerome Pouiller wrote: > > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > > > index 5de9ccf02285..aff0559653bf 100644 > > > --- a/drivers/staging/wfx/sta.c > > > +++ b/drivers/staging/wfx/sta.c > > > @@ -154,18 +154,26 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, > > > bool *enable_ps) > > > chan0 = wdev_to_wvif(wvif->wdev, > > > 0)->vif->bss_conf.chandef.chan; > > > if (wdev_to_wvif(wvif->wdev, 1)) > > > chan1 = wdev_to_wvif(wvif->wdev, > > > 1)->vif->bss_conf.chandef.chan; > > > - if (chan0 && chan1 && chan0->hw_value != chan1->hw_value && > > > - wvif->vif->type != NL80211_IFTYPE_AP) { > > > - // It is necessary to enable powersave if channels > > > - // are different. > > > - if (enable_ps) > > > - *enable_ps = true; > > > - if (wvif->wdev->force_ps_timeout > -1) > > > - return wvif->wdev->force_ps_timeout; > > > - else if (wfx_api_older_than(wvif->wdev, 3, 2)) > > > - return 0; > > > - else > > > - return 30; > > > + if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { > > > + if (chan0->hw_value == chan1->hw_value) { > > > + // It is useless to enable PS if channels are the > > > same. > > > + if (enable_ps) > > > + *enable_ps = false; > > > + if (wvif->vif->bss_conf.assoc && > > > wvif->vif->bss_conf.ps) > > > + dev_info(wvif->wdev->dev, "ignoring > > > requested PS mode"); > > > + return -1; > > > > I can't be happy about this -1 return or how it's handled in the caller. > > There is already a -1 return so it's not really a new bug, though... > > I see what you mean. However, I remember it is easy to break things > here and I don't want to change that in a rush. So, I would prefer to > solve that in a further PR. Yes. That's fine. The return -1 was already there. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/33] staging: wfx: ignore PS when STA/AP share same channel
On Monday 13 September 2021 11:33:28 CEST Dan Carpenter wrote: > On Mon, Sep 13, 2021 at 10:30:15AM +0200, Jerome Pouiller wrote: > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > > index 5de9ccf02285..aff0559653bf 100644 > > --- a/drivers/staging/wfx/sta.c > > +++ b/drivers/staging/wfx/sta.c > > @@ -154,18 +154,26 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, > > bool *enable_ps) > > chan0 = wdev_to_wvif(wvif->wdev, > > 0)->vif->bss_conf.chandef.chan; > > if (wdev_to_wvif(wvif->wdev, 1)) > > chan1 = wdev_to_wvif(wvif->wdev, > > 1)->vif->bss_conf.chandef.chan; > > - if (chan0 && chan1 && chan0->hw_value != chan1->hw_value && > > - wvif->vif->type != NL80211_IFTYPE_AP) { > > - // It is necessary to enable powersave if channels > > - // are different. > > - if (enable_ps) > > - *enable_ps = true; > > - if (wvif->wdev->force_ps_timeout > -1) > > - return wvif->wdev->force_ps_timeout; > > - else if (wfx_api_older_than(wvif->wdev, 3, 2)) > > - return 0; > > - else > > - return 30; > > + if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { > > + if (chan0->hw_value == chan1->hw_value) { > > + // It is useless to enable PS if channels are the > > same. > > + if (enable_ps) > > + *enable_ps = false; > > + if (wvif->vif->bss_conf.assoc && > > wvif->vif->bss_conf.ps) > > + dev_info(wvif->wdev->dev, "ignoring requested > > PS mode"); > > + return -1; > > I can't be happy about this -1 return or how it's handled in the caller. > There is already a -1 return so it's not really a new bug, though... I see what you mean. However, I remember it is easy to break things here and I don't want to change that in a rush. So, I would prefer to solve that in a further PR. -- Jérôme Pouiller ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 03/33] staging: wfx: ignore PS when STA/AP share same channel
On Mon, Sep 13, 2021 at 10:30:15AM +0200, Jerome Pouiller wrote: > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > index 5de9ccf02285..aff0559653bf 100644 > --- a/drivers/staging/wfx/sta.c > +++ b/drivers/staging/wfx/sta.c > @@ -154,18 +154,26 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, > bool *enable_ps) > chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan; > if (wdev_to_wvif(wvif->wdev, 1)) > chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan; > - if (chan0 && chan1 && chan0->hw_value != chan1->hw_value && > - wvif->vif->type != NL80211_IFTYPE_AP) { > - // It is necessary to enable powersave if channels > - // are different. > - if (enable_ps) > - *enable_ps = true; > - if (wvif->wdev->force_ps_timeout > -1) > - return wvif->wdev->force_ps_timeout; > - else if (wfx_api_older_than(wvif->wdev, 3, 2)) > - return 0; > - else > - return 30; > + if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { > + if (chan0->hw_value == chan1->hw_value) { > + // It is useless to enable PS if channels are the same. > + if (enable_ps) > + *enable_ps = false; > + if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps) > + dev_info(wvif->wdev->dev, "ignoring requested > PS mode"); > + return -1; I can't be happy about this -1 return or how it's handled in the caller. There is already a -1 return so it's not really a new bug, though... regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 03/33] staging: wfx: ignore PS when STA/AP share same channel
From: Jérôme Pouiller When multiple interface are in use. One is always AP while the other is always station. When the two interface use the same channel, it makes no sense to enabled Power Saving (PS) on the station. Indeed, because of the AP, the device will be kept awake on this channel anyway. In add, when multiple interface are in use, mac80211 does not update the PS information and delegate to the driver responsibility to do the right thing. Thus, in the current code, when the user enable PS in this configuration, the driver finally enable PS-Poll which is probably not what the user expected. This patch detect this case and applies a sane configuration in all cases. Signed-off-by: Jérôme Pouiller --- drivers/staging/wfx/sta.c | 32 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c index 5de9ccf02285..aff0559653bf 100644 --- a/drivers/staging/wfx/sta.c +++ b/drivers/staging/wfx/sta.c @@ -154,18 +154,26 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps) chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan; if (wdev_to_wvif(wvif->wdev, 1)) chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan; - if (chan0 && chan1 && chan0->hw_value != chan1->hw_value && - wvif->vif->type != NL80211_IFTYPE_AP) { - // It is necessary to enable powersave if channels - // are different. - if (enable_ps) - *enable_ps = true; - if (wvif->wdev->force_ps_timeout > -1) - return wvif->wdev->force_ps_timeout; - else if (wfx_api_older_than(wvif->wdev, 3, 2)) - return 0; - else - return 30; + if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { + if (chan0->hw_value == chan1->hw_value) { + // It is useless to enable PS if channels are the same. + if (enable_ps) + *enable_ps = false; + if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps) + dev_info(wvif->wdev->dev, "ignoring requested PS mode"); + return -1; + } else { + // It is necessary to enable PS if channels + // are different. + if (enable_ps) + *enable_ps = true; + if (wvif->wdev->force_ps_timeout > -1) + return wvif->wdev->force_ps_timeout; + else if (wfx_api_older_than(wvif->wdev, 3, 2)) + return 0; + else + return 30; + } } if (enable_ps) *enable_ps = wvif->vif->bss_conf.ps; -- 2.33.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel