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] [day] [month] [year] [list]

Message-ID: <20220418045610.GB1127014@jaehee-ThinkPad-X1-Extreme>
Date:   Mon, 18 Apr 2022 00:56:10 -0400
From:   Jaehee Park <jhpark1013@...il.com>
To:     Jérôme Pouiller <jerome.pouiller@...abs.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev, Stefano Brivio <sbrivio@...hat.com>
Subject: Re: [PATCH V3] wfx: use container_of() to get vif

On Wed, Apr 13, 2022 at 09:26:45AM +0200, Jérôme Pouiller wrote:
> 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
> 
> 
>

Hi Jérôme, Sorry about the delay! I have sent a revised patch to the
wireless-next mailing list for your review.
Thanks,
Jaehee