linux-kernel - Re: [PATCH V3] wfx: use container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Apr 2022 09:26:45 +0200
From:   Jérôme Pouiller <jerome.pouiller@...abs.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev, Jaehee Park <jhpark1013@...il.com>,
        Stefano Brivio <sbrivio@...hat.com>
Subject: Re: [PATCH V3] wfx: use container_of() to get vif

Hello Jaehee,

On Tuesday 12 April 2022 06:12:18 CEST Jaehee Park wrote:
> 
> Currently, upon virtual interface creation, wfx_add_interface() stores
> a reference to the corresponding struct ieee80211_vif in private data,
> for later usage. This is not needed when using the container_of
> construct. This construct already has all the info it needs to retrieve
> the reference to the corresponding struct from the offset that is
> already available, inherent in container_of(), between its type and
> member inputs (struct ieee80211_vif and drv_priv, respectively).
> Remove vif (which was previously storing the reference to the struct
> ieee80211_vif) from the struct wfx_vif, define a macro
> wvif_to_vif(wvif) for container_of(), and replace all wvif->vif with
> the newly defined container_of construct.
> 
> Signed-off-by: Jaehee Park <jhpark1013@...il.com>
> ---
> 
> Changes in v3:
> - Made edits to the commit message.
> - Shortened the macro name from wvif_to_vif to to_vif.

hmm... wvif_to_vif() was fine for me (while to_vif() is a bit too
generic).

> - For functions that had more than one instance of vif, defined one
> reference vif at the beginning of the function and used that instead.

I suggest to declare a new variable even if there is only one instance
of vif. So:
  - the code is unified
  - it avoids the syntax foo(var)->bar

> - Broke the if-statements that ran long into two lines.
> (There are 3 lines that exceed 80 by less than 4 characters. Two of
> those lines of code could be shorted but it involved defining two more
> variables, and could potentially make the code less readable.)
> 
> Note: I will mail this patch to the wireless-next tree after testing.
> 
> 
>  drivers/staging/wfx/wfx.h     |  2 +-
>  drivers/staging/wfx/data_rx.c |  5 ++--
>  drivers/staging/wfx/data_tx.c |  2 +-
>  drivers/staging/wfx/key.c     |  2 +-
>  drivers/staging/wfx/queue.c   |  2 +-
>  drivers/staging/wfx/scan.c    | 10 +++++---
>  drivers/staging/wfx/sta.c     | 46 +++++++++++++++++++----------------
>  7 files changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> index 78f2a416fe4f..a07baadc5e70 100644
> --- a/drivers/staging/wfx/wfx.h
> +++ b/drivers/staging/wfx/wfx.h
> @@ -25,7 +25,7 @@
>  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
>  #define USEC_PER_TU 1024
> 
> -#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> +#define to_vif(wvif)container_of((void *)wvif, struct ieee80211_vif, drv_priv)

It seems you have stacked up this v3 on your v2. So this v3 does not apply
on any public tree. You have to merge your v2 and your v3.

> 
>  struct wfx_hwbus_ops;
> 
> diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
> index 98c2223089b8..3677b3431467 100644
> --- a/drivers/staging/wfx/data_rx.c
> +++ b/drivers/staging/wfx/data_rx.c

The wfx driver has moved, you have to rebase your work.

[...]

-- 
Jérôme Pouiller