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

From: Jaehee Park
Date: Mon Apr 11 2022 - 23:57:51 EST


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
>