| Message ID | 20210910160504.1794332-3-Jerome.Pouiller@silabs.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | staging/wfx: usual maintenance | expand |
On Fri, Sep 10, 2021 at 06:04:35PM +0200, Jerome Pouiller wrote: > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > During the scan requests, the Tx traffic is suspended. This lock is > shared by all the network interfaces. So, a scan request on one > interface will block the traffic on a second interface. This causes > trouble when the queued traffic contains CAB (Content After DTIM Beacon) > since this traffic cannot be delayed. > > It could be possible to make the lock local to each interface. But It > would only push the problem further. The device won't be able to send > the CAB before the end of the scan. > > So, this patch just ignore the DTIM indication when a scan is in > progress. The firmware will send another indication on the next DTIM and > this time the system will be able to send the traffic just behind the > beacon. > > The only drawback of this solution is that the stations connected to > the AP will wait for traffic after the DTIM for nothing. But since the > case is really rare it is not a big deal. > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > --- > drivers/staging/wfx/sta.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > index a236e5bb6914..d901588237a4 100644 > --- a/drivers/staging/wfx/sta.c > +++ b/drivers/staging/wfx/sta.c > @@ -629,8 +629,18 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set) > > void wfx_suspend_resume_mc(struct wfx_vif *wvif, enum sta_notify_cmd notify_cmd) > { > + struct wfx_vif *wvif_it; > + > if (notify_cmd != STA_NOTIFY_AWAKE) > return; > + > + // Device won't be able to honor CAB if a scan is in progress on any > + // interface. Prefer to skip this DTIM and wait for the next one. In one patch you drop // comments but you introduce some of your self. > + wvif_it = NULL; > + while ((wvif_it = wvif_iterate(wvif->wdev, wvif_it)) != NULL) > + if (mutex_is_locked(&wvif_it->scan_lock)) > + return; > + > if (!wfx_tx_queues_has_cab(wvif) || wvif->after_dtim_tx_allowed) > dev_warn(wvif->wdev->dev, "incorrect sequence (%d CAB in queue)", > wfx_tx_queues_has_cab(wvif)); > -- > 2.33.0 >
On Friday 10 September 2021 18:31:00 CEST Kari Argillander wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On Fri, Sep 10, 2021 at 06:04:35PM +0200, Jerome Pouiller wrote: > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > During the scan requests, the Tx traffic is suspended. This lock is > > shared by all the network interfaces. So, a scan request on one > > interface will block the traffic on a second interface. This causes > > trouble when the queued traffic contains CAB (Content After DTIM Beacon) > > since this traffic cannot be delayed. > > > > It could be possible to make the lock local to each interface. But It > > would only push the problem further. The device won't be able to send > > the CAB before the end of the scan. > > > > So, this patch just ignore the DTIM indication when a scan is in > > progress. The firmware will send another indication on the next DTIM and > > this time the system will be able to send the traffic just behind the > > beacon. > > > > The only drawback of this solution is that the stations connected to > > the AP will wait for traffic after the DTIM for nothing. But since the > > case is really rare it is not a big deal. > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > --- > > drivers/staging/wfx/sta.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > > index a236e5bb6914..d901588237a4 100644 > > --- a/drivers/staging/wfx/sta.c > > +++ b/drivers/staging/wfx/sta.c > > @@ -629,8 +629,18 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set) > > > > void wfx_suspend_resume_mc(struct wfx_vif *wvif, enum sta_notify_cmd notify_cmd) > > { > > + struct wfx_vif *wvif_it; > > + > > if (notify_cmd != STA_NOTIFY_AWAKE) > > return; > > + > > + // Device won't be able to honor CAB if a scan is in progress on any > > + // interface. Prefer to skip this DTIM and wait for the next one. > > In one patch you drop // comments but you introduce some of your self. Indeed. When I wrote this patch, I didn't yet care to this issue. Is it a big deal since it is fixed in patch 27?
On Fri, Sep 10, 2021 at 06:54:36PM +0200, Jérôme Pouiller wrote: > On Friday 10 September 2021 18:31:00 CEST Kari Argillander wrote: > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > On Fri, Sep 10, 2021 at 06:04:35PM +0200, Jerome Pouiller wrote: > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > During the scan requests, the Tx traffic is suspended. This lock is > > > shared by all the network interfaces. So, a scan request on one > > > interface will block the traffic on a second interface. This causes > > > trouble when the queued traffic contains CAB (Content After DTIM Beacon) > > > since this traffic cannot be delayed. > > > > > > It could be possible to make the lock local to each interface. But It > > > would only push the problem further. The device won't be able to send > > > the CAB before the end of the scan. > > > > > > So, this patch just ignore the DTIM indication when a scan is in > > > progress. The firmware will send another indication on the next DTIM and > > > this time the system will be able to send the traffic just behind the > > > beacon. > > > > > > The only drawback of this solution is that the stations connected to > > > the AP will wait for traffic after the DTIM for nothing. But since the > > > case is really rare it is not a big deal. > > > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > --- > > > drivers/staging/wfx/sta.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > > > index a236e5bb6914..d901588237a4 100644 > > > --- a/drivers/staging/wfx/sta.c > > > +++ b/drivers/staging/wfx/sta.c > > > @@ -629,8 +629,18 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set) > > > > > > void wfx_suspend_resume_mc(struct wfx_vif *wvif, enum sta_notify_cmd notify_cmd) > > > { > > > + struct wfx_vif *wvif_it; > > > + > > > if (notify_cmd != STA_NOTIFY_AWAKE) > > > return; > > > + > > > + // Device won't be able to honor CAB if a scan is in progress on any > > > + // interface. Prefer to skip this DTIM and wait for the next one. > > > > In one patch you drop // comments but you introduce some of your self. > > Indeed. When I wrote this patch, I didn't yet care to this issue. Is it > a big deal since it is fixed in patch 27? No for me. Just little detail. > > > > -- > Jérôme Pouiller > >
On Friday 10 September 2021 19:01:05 CEST Kari Argillander wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On Fri, Sep 10, 2021 at 06:54:36PM +0200, Jérôme Pouiller wrote: > > On Friday 10 September 2021 18:31:00 CEST Kari Argillander wrote: > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > > > > On Fri, Sep 10, 2021 at 06:04:35PM +0200, Jerome Pouiller wrote: > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > > > During the scan requests, the Tx traffic is suspended. This lock is > > > > shared by all the network interfaces. So, a scan request on one > > > > interface will block the traffic on a second interface. This causes > > > > trouble when the queued traffic contains CAB (Content After DTIM Beacon) > > > > since this traffic cannot be delayed. > > > > > > > > It could be possible to make the lock local to each interface. But It > > > > would only push the problem further. The device won't be able to send > > > > the CAB before the end of the scan. > > > > > > > > So, this patch just ignore the DTIM indication when a scan is in > > > > progress. The firmware will send another indication on the next DTIM and > > > > this time the system will be able to send the traffic just behind the > > > > beacon. > > > > > > > > The only drawback of this solution is that the stations connected to > > > > the AP will wait for traffic after the DTIM for nothing. But since the > > > > case is really rare it is not a big deal. > > > > > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > --- > > > > drivers/staging/wfx/sta.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c > > > > index a236e5bb6914..d901588237a4 100644 > > > > --- a/drivers/staging/wfx/sta.c > > > > +++ b/drivers/staging/wfx/sta.c > > > > @@ -629,8 +629,18 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set) > > > > > > > > void wfx_suspend_resume_mc(struct wfx_vif *wvif, enum sta_notify_cmd notify_cmd) > > > > { > > > > + struct wfx_vif *wvif_it; > > > > + > > > > if (notify_cmd != STA_NOTIFY_AWAKE) > > > > return; > > > > + > > > > + // Device won't be able to honor CAB if a scan is in progress on any > > > > + // interface. Prefer to skip this DTIM and wait for the next one. > > > > > > In one patch you drop // comments but you introduce some of your self. > > > > Indeed. When I wrote this patch, I didn't yet care to this issue. Is it > > a big deal since it is fixed in patch 27? > > No for me. Just little detail. OK, right.
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c index a236e5bb6914..d901588237a4 100644 --- a/drivers/staging/wfx/sta.c +++ b/drivers/staging/wfx/sta.c @@ -629,8 +629,18 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set) void wfx_suspend_resume_mc(struct wfx_vif *wvif, enum sta_notify_cmd notify_cmd) { + struct wfx_vif *wvif_it; + if (notify_cmd != STA_NOTIFY_AWAKE) return; + + // Device won't be able to honor CAB if a scan is in progress on any + // interface. Prefer to skip this DTIM and wait for the next one. + wvif_it = NULL; + while ((wvif_it = wvif_iterate(wvif->wdev, wvif_it)) != NULL) + if (mutex_is_locked(&wvif_it->scan_lock)) + return; + if (!wfx_tx_queues_has_cab(wvif) || wvif->after_dtim_tx_allowed) dev_warn(wvif->wdev->dev, "incorrect sequence (%d CAB in queue)", wfx_tx_queues_has_cab(wvif));