Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 11 Apr 2022 23:57:39 -0400
From:   Jaehee Park <jhpark1013@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     =?iso-8859-1?B?Suly9G1l?= Pouiller <jerome.pouiller@silabs.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        outreachy@lists.linux.dev, Stefano Brivio <sbrivio@redhat.com>
Subject: Re: [PATCH v2] staging: wfx: use container_of() to get vif
Message-ID: <20220412035739.GA2856917@jaehee-ThinkPad-X1-Extreme>
References: <20220408032349.GA2132476@jaehee-ThinkPad-X1-Extreme>
 <20220408061936.GP3293@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220408061936.GP3293@kadam>
Precedence: bulk

On Fri, Apr 08, 2022 at 09:19:36AM +0300, Dan Carpenter wrote:
> On Thu, Apr 07, 2022 at 11:23:49PM -0400, Jaehee Park wrote:
> > diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> > index 6594cc647c2f..78f2a416fe4f 100644
> > --- a/drivers/staging/wfx/wfx.h
> > +++ b/drivers/staging/wfx/wfx.h
> > @@ -25,6 +25,8 @@
> >  #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))
> > +
> 
> Better to make this a function.
> 

Hi Dan, Thank you for your comments. To make sure I'm understanding your
concerns correctly, do you mean I should define a function instead of
using this macros here?

> Stefano's comments are correct.  It would have saved space with the 80
> limit to do a "struct ieee80211_vif *vif = wvif_to_vif();" at the start

Got it. I implemented this on the next patch (v3) that I will be sending
out soon. 

> of the function.  Also dereferencing the results of a function call
> like this, "frob(foo)->bar", without checking makes me itch.  If it's
> at the top of the function then that's kind of different.  I normally
> assume that the functions in the declaration block cannot fail.  From
> analysing static checker warnings, putting functions which can fail in
> that the declaration block is risky.
> 

The frob(foo)->bar in my case would be wvif_to_vif(wvif)->type?

> It's always better to test things but this patch looks correct to me:
> 
> The add interface does:
> 
> 	struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv
>         ...
> 	wvif->vif = vif;
> 
> The remove interface does:
> 	wvif->vif = NULL;
> 
> Those are the only places where ->vif is set container_of() will always
> work.

Yes this is true. The add and remove interface was the inspiration point
for this patch.
> 
> regards,
> dan carpenter
>