| Message ID | 20210910160504.1794332-1-Jerome.Pouiller@silabs.com |
|---|---|
| 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 Fri, Sep 10, 2021 at 06:49:30PM +0200, Jérôme Pouiller wrote: > On Friday 10 September 2021 18:27:18 CEST Kari Argillander wrote: > > On Fri, Sep 10, 2021 at 06:05:02PM +0200, Jerome Pouiller wrote: > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > Comments after the last #endif of header files don't bring any > > > information and are redundant with the name of the file. Drop them. > > > > How so? You see right away that this indeed is header guard and not some > > other random thing. Also kernel coding standard says: > > > > At the end of any non-trivial #if or #ifdef block (more than a > > few line), place a comment after the #endif on the same line, > > noting the conditional expression used. > > > > There is no point dropping them imo. If you think about space saving > > this patch will take more space. Because it will be in version history. > > So nack from me unless some one can trun my head around. > > IMHO, the #endif on the last line of an header file terminates a trivial > #ifdef block. > Moreover, they are often out-of-sync with the #ifndef statement, like here: That one is of course true. > > [...] > > > diff --git a/drivers/staging/wfx/key.h b/drivers/staging/wfx/key.h > > > index dd189788acf1..2d135eff7af2 100644 > > > --- a/drivers/staging/wfx/key.h > > > +++ b/drivers/staging/wfx/key.h > > > @@ -17,4 +17,4 @@ int wfx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, > > > struct ieee80211_vif *vif, struct ieee80211_sta *sta, > > > struct ieee80211_key_conf *key); > > > > > > -#endif /* WFX_STA_H */ > > > +#endif > [...] > > -- > Jérôme Pouiller > >
On Friday 10 September 2021 18:57:43 CEST Kari Argillander wrote: > > On Fri, Sep 10, 2021 at 06:05:04PM +0200, Jerome Pouiller wrote: > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > Function arguments must be aligned with left parenthesis. Apply that > > rule. > > To my eyes something still go wrong with this patch. Might be my email > fault, but every other patch looks ok. Now these are too left. I don't try anymore to check alignments with my email viewer. The original patch is as I expect (and I take care to send my patch with base64 to avoid pitfalls with MS Exchange). So, I think the is correct. > Also it should alight with first argument not left parenthesis? Absolutely.
On Friday 10 September 2021 19:07:41 CEST Kari Argillander wrote: > > On Fri, Sep 10, 2021 at 06:04:33PM +0200, Jerome Pouiller wrote: > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > Hi, > > > > The following PR contains now usual maintenance for the wfx driver. I have > > more-or-less sorted the patches by importance: > > - the first ones are fixes for a few corner-cases reported by users > > - the patches 9 and 10 add support for CSA and TDLS > > - then the end of the series is mostly cosmetics and nitpicking > > Nicely formated patch series. Should be pretty easy to review. I just > check for fast eyes. But overall nice clean up series. Thank you for the review. Monday, I will send a v2 with the small changes you mentioned.
From: Jérôme Pouiller <jerome.pouiller@silabs.com> Hi, The following PR contains now usual maintenance for the wfx driver. I have more-or-less sorted the patches by importance: - the first ones are fixes for a few corner-cases reported by users - the patches 9 and 10 add support for CSA and TDLS - then the end of the series is mostly cosmetics and nitpicking I have wait longer than I initially wanted before to send this PR. It is because didn't want to conflict with the PR currently in review[1] to relocate this driver into the main tree. However, this PR started to be very large and nothing seems to move on main-tree side so I decided to not wait longer. Kalle, I am going to send a new version of [1] as soon as this PR will be accepted. I hope you will have time to review it one day :-). [1] https://lore.kernel.org/all/20210315132501.441681-1-Jerome.Pouiller@silabs.com/ Jérôme Pouiller (31): staging: wfx: use abbreviated message for "incorrect sequence" staging: wfx: do not send CAB while scanning staging: wfx: ignore PS when STA/AP share same channel staging: wfx: wait for SCAN_CMPL after a SCAN_STOP staging: wfx: avoid possible lock-up during scan staging: wfx: drop unused argument from hif_scan() staging: wfx: fix atomic accesses in wfx_tx_queue_empty() staging: wfx: take advantage of wfx_tx_queue_empty() staging: wfx: declare support for TDLS staging: wfx: fix support for CSA staging: wfx: relax the PDS existence constraint staging: wfx: simplify API coherency check staging: wfx: update with API 3.8 staging: wfx: uniformize counter names staging: wfx: fix misleading 'rate_id' usage staging: wfx: declare variables at beginning of functions staging: wfx: simplify hif_join() staging: wfx: reorder function for slightly better eye candy staging: wfx: fix error names staging: wfx: apply naming rules in hif_tx_mib.c staging: wfx: remove unused definition staging: wfx: remove useless debug statement staging: wfx: fix space after cast operator staging: wfx: remove references to WFxxx in comments staging: wfx: update files descriptions staging: wfx: reformat comment staging: wfx: avoid c99 comments staging: wfx: fix comments styles staging: wfx: remove useless comments after #endif staging: wfx: explain the purpose of wfx_send_pds() staging: wfx: indent functions arguments drivers/staging/wfx/bh.c | 33 +++---- drivers/staging/wfx/bh.h | 4 +- drivers/staging/wfx/bus_sdio.c | 8 +- drivers/staging/wfx/bus_spi.c | 22 ++--- drivers/staging/wfx/data_rx.c | 7 +- drivers/staging/wfx/data_rx.h | 4 +- drivers/staging/wfx/data_tx.c | 87 +++++++++-------- drivers/staging/wfx/data_tx.h | 6 +- drivers/staging/wfx/debug.c | 54 ++++++----- drivers/staging/wfx/debug.h | 2 +- drivers/staging/wfx/fwio.c | 26 ++--- drivers/staging/wfx/fwio.h | 2 +- drivers/staging/wfx/hif_api_cmd.h | 14 +-- drivers/staging/wfx/hif_api_general.h | 25 ++--- drivers/staging/wfx/hif_api_mib.h | 85 ++++++++-------- drivers/staging/wfx/hif_rx.c | 23 ++--- drivers/staging/wfx/hif_rx.h | 3 +- drivers/staging/wfx/hif_tx.c | 61 +++++------- drivers/staging/wfx/hif_tx.h | 6 +- drivers/staging/wfx/hif_tx_mib.c | 14 +-- drivers/staging/wfx/hif_tx_mib.h | 2 +- drivers/staging/wfx/hwio.c | 6 +- drivers/staging/wfx/hwio.h | 20 ++-- drivers/staging/wfx/key.c | 30 +++--- drivers/staging/wfx/key.h | 4 +- drivers/staging/wfx/main.c | 39 +++++--- drivers/staging/wfx/main.h | 3 +- drivers/staging/wfx/queue.c | 43 ++++---- drivers/staging/wfx/queue.h | 6 +- drivers/staging/wfx/scan.c | 55 +++++++---- drivers/staging/wfx/scan.h | 4 +- drivers/staging/wfx/sta.c | 135 +++++++++++++++----------- drivers/staging/wfx/sta.h | 8 +- drivers/staging/wfx/traces.h | 2 +- drivers/staging/wfx/wfx.h | 14 ++- 35 files changed, 457 insertions(+), 400 deletions(-)