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 >
