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, 10:02 a.m. UTC | #1 
On Mon, Sep 13, 2021 at 10:30:24AM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> The 'channel' argument of hif_join() should never be NULL. hif_join()
> does not have the responsibility to recover bug of caller. A call to
> WARN() at the beginning of the function reminds this constraint to the
> developer.
> 
> In current code, if the argument channel is NULL, memory leaks. The new
> code just emit a warning and does not give the illusion that it is
> supported (and indeed a Oops will probably raise a few lines below).
> 
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  drivers/staging/wfx/hif_tx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> index 14b7e047916e..6ffbae32028b 100644
> --- a/drivers/staging/wfx/hif_tx.c
> +++ b/drivers/staging/wfx/hif_tx.c
> @@ -299,10 +299,9 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
>  
>  	WARN_ON(!conf->beacon_int);
>  	WARN_ON(!conf->basic_rates);
> +	WARN_ON(!channel);

This fine.  I'm not trying to make people redo their patches especially
when you're doing a great job as a maintainer.

But generally these WARN_ON()s are pointless.  It's never going to
happen and if we try to handle all the thing which will not happen that's
an impossible task.  But specificically with NULL dereferences, the
WARN() will generate a stack trace and also the Oops will generate a
stack trace.  It's duplicative.

regards,
dan carpenter