Re: [PATCH v2] staging: wfx: use container_of() to get vif

[Jaehee Park <jhpark1013@gmail.com>]

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
>