mbox series

[v2,00/33] staging/wfx: usual maintenance

Message ID 20210913083045.1881321-1-Jerome.Pouiller@silabs.com
Headers show
Series staging/wfx: usual maintenance | expand

Message

Jérôme Pouiller Sept. 13, 2021, 8:30 a.m. UTC
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 and the two last 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/

v2:
  - Add patches 32 and 33 to solve a possible race when device is
    misconfigured
  - Fix C99 comments (Kari)
  - Replace "API 3.8" by "firmware API 3.8" (Kari)
  - Fix wording "aligned with first argument" instead of "aligned with
    opening parenthesis"

Jérôme Pouiller (33):
  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 the firmware 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
  staging: wfx: ensure IRQ is ready before enabling it
  staging: wfx: early exit of PDS is not correct

 drivers/staging/wfx/bh.c              |  33 +++----
 drivers/staging/wfx/bh.h              |   4 +-
 drivers/staging/wfx/bus_sdio.c        |  29 +++---
 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            |  37 +++++--
 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, 469 insertions(+), 407 deletions(-)

Comments

Dan Carpenter Sept. 13, 2021, 9:58 a.m. UTC | #1
On Mon, Sep 13, 2021 at 10:30:23AM +0200, Jerome Pouiller wrote:
> @@ -395,9 +395,7 @@ int wfx_probe(struct wfx_dev *wdev)
>  
>  	dev_dbg(wdev->dev, "sending configuration file %s\n",
>  		wdev->pdata.file_pds);
> -	err = wfx_send_pdata_pds(wdev);
> -	if (err < 0)
> -		goto err0;
> +	wfx_send_pdata_pds(wdev);

You revert this change in patch 33 so let's drop this and 33 both.

regards,
dan carpenter
Dan Carpenter Sept. 13, 2021, 10:06 a.m. UTC | #2
On Mon, Sep 13, 2021 at 10:30:25AM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> The firmware API 3.8 introduces new statistic counters. These changes
> are backward compatible.
> 
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  drivers/staging/wfx/debug.c       | 3 +++
>  drivers/staging/wfx/hif_api_mib.h | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wfx/debug.c b/drivers/staging/wfx/debug.c
> index eedada78c25f..e67ca0d818ba 100644
> --- a/drivers/staging/wfx/debug.c
> +++ b/drivers/staging/wfx/debug.c
> @@ -109,6 +109,9 @@ static int wfx_counters_show(struct seq_file *seq, void *v)
>  
>  	PUT_COUNTER(rx_beacon);
>  	PUT_COUNTER(miss_beacon);
> +	PUT_COUNTER(rx_dtim);
> +	PUT_COUNTER(rx_dtim_aid0_clr);
> +	PUT_COUNTER(rx_dtim_aid0_set);
>  
>  #undef PUT_COUNTER

Not related to the patch but the PUT_COUNTER macro should be called
something like PRINT_COUNTER.  It's not a get/put API.

regards,
dan carpenter
Dan Carpenter Sept. 13, 2021, 10:42 a.m. UTC | #3
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