diff mbox series

[02/31] staging: wfx: do not send CAB while scanning

Message ID 20210910160504.1794332-3-Jerome.Pouiller@silabs.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series staging/wfx: usual maintenance | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Jérôme Pouiller Sept. 10, 2021, 4:04 p.m. UTC 
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(+)

Comments

Kari Argillander Sept. 10, 2021, 4:31 p.m. UTC | #1 
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
>
Jérôme Pouiller Sept. 10, 2021, 4:54 p.m. UTC | #2 
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?
Kari Argillander Sept. 10, 2021, 5:01 p.m. UTC | #3 
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
> 
>
Jérôme Pouiller Sept. 10, 2021, 5:12 p.m. UTC | #4 
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 mbox series

Patch

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));