| Message ID | 20220418035110.GA937332@jaehee-ThinkPad-X1-Extreme (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wfx: use container_of() to get vif | expand |
On Monday 18 April 2022 05:51:10 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 function > 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@gmail.com> > --- > > Changes from staging to wireless-next tree > - changed macro into function and named it back to wvif_to_vif > - fit all lines in patch to 80 columns > - decared a reference to vif at the beginning of the functions > > NOTE: Jérôme is going to be testing this patch on his hardware Don't forget to increment the version number of your submission (option -v of git send-email). > drivers/net/wireless/silabs/wfx/wfx.h | 6 +- > drivers/net/wireless/silabs/wfx/data_rx.c | 5 +- > drivers/net/wireless/silabs/wfx/data_tx.c | 3 +- > drivers/net/wireless/silabs/wfx/key.c | 4 +- > drivers/net/wireless/silabs/wfx/queue.c | 3 +- > drivers/net/wireless/silabs/wfx/scan.c | 9 ++- > drivers/net/wireless/silabs/wfx/sta.c | 69 ++++++++++++++--------- > 7 files changed, 63 insertions(+), 36 deletions(-) > > diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h > index 6594cc647c2f..718693a4273d 100644 > --- a/drivers/net/wireless/silabs/wfx/wfx.h > +++ b/drivers/net/wireless/silabs/wfx/wfx.h > @@ -61,7 +61,6 @@ struct wfx_dev { > > struct wfx_vif { > struct wfx_dev *wdev; > - struct ieee80211_vif *vif; > struct ieee80211_channel *channel; > int id; > > @@ -91,6 +90,11 @@ struct wfx_vif { > struct completion set_pm_mode_complete; > }; > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > +{ > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > +} > + > static inline struct wfx_vif *wdev_to_wvif(struct wfx_dev *wdev, int vif_id) > { > if (vif_id >= ARRAY_SIZE(wdev->vif)) { > diff --git a/drivers/net/wireless/silabs/wfx/data_rx.c b/drivers/net/wireless/silabs/wfx/data_rx.c > index a4b5ffe158e4..342b9cd0e74c 100644 > --- a/drivers/net/wireless/silabs/wfx/data_rx.c > +++ b/drivers/net/wireless/silabs/wfx/data_rx.c > @@ -16,6 +16,7 @@ > static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt) > { > int params, tid; > + struct ieee80211_vif *vif = wvif_to_vif(wvif); When you can, try to place the longest declaration first ("reverse Christmas tree order"). [...] > diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c > index 3297d73c327a..97fcbad23c94 100644 > --- a/drivers/net/wireless/silabs/wfx/sta.c > +++ b/drivers/net/wireless/silabs/wfx/sta.c [...] > @@ -152,19 +153,28 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps) > { > struct ieee80211_channel *chan0 = NULL, *chan1 = NULL; > struct ieee80211_conf *conf = &wvif->wdev->hw->conf; > + struct ieee80211_vif *vif = wvif_to_vif(wvif); > > - WARN(!wvif->vif->bss_conf.assoc && enable_ps, > + WARN(!vif->bss_conf.assoc && enable_ps, > "enable_ps is reliable only if associated"); > - if (wdev_to_wvif(wvif->wdev, 0)) > - chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan; > - if (wdev_to_wvif(wvif->wdev, 1)) > - chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan; > - if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { > + if (wdev_to_wvif(wvif->wdev, 0)) { > + struct wfx_vif *wvif_ch0 = wdev_to_wvif(wvif->wdev, 0); > + struct ieee80211_vif *vif_ch0 = wvif_to_vif(wvif_ch0); > + > + chan0 = vif_ch0->bss_conf.chandef.chan; > + } > + if (wdev_to_wvif(wvif->wdev, 1)) { > + struct wfx_vif *wvif_ch1 = wdev_to_wvif(wvif->wdev, 1); > + struct ieee80211_vif *vif_ch1 = wvif_to_vif(wvif_ch1); > + > + chan1 = vif_ch1->bss_conf.chandef.chan; > + } I think this code could be simplified into: if (wvif->wdev->vif[1]) chan1 = wvif->wdev->vif[1]->bss_conf.chandef.chan; (If you choose this way, I suggest to place this change in a separate patch) [...]
Jaehee Park <jhpark1013@gmail.com> writes: > 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 function > 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@gmail.com> [...] > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > +{ > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > +} Why the void pointer cast? Avoid casts as much possible.
On mercoledì 20 aprile 2022 13:57:57 CEST Kalle Valo wrote: > Jaehee Park <jhpark1013@gmail.com> writes: > > > 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 function > > 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@gmail.com> > > [...] > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > > +{ > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > > +} > > Why the void pointer cast? Avoid casts as much possible. In a previous email Jaehee wrote that she could compile her changes only by using that "(void *)" cast. I replied that probably this is a hint that something is broken, although my argument is not necessarily a "proof". Might very well be that this cast was needed in this particular situation but I cannot see why. @Jaehee, please try to explain why this "(void *)" cast is actually necessary and why your changes cannot avoid it. Thanks, Fabio M. De Francesco
On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote: > > Jaehee Park <jhpark1013@gmail.com> writes: > > > 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 function > > 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@gmail.com> > > [...] > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > > +{ > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > > +} > > Why the void pointer cast? Avoid casts as much possible. > Hi Kalle, Sorry for the delay in getting back to you about why the void pointer cast was used. In essence, I'm taking private data with a driver-specific pointer and that needs to be resolved back to a generic pointer. The private data (drv_priv) is declared as a generic u8 array in struct ieee80211_vif, but wvif is a more specific type. I wanted to also point to existing, reasonable examples such as: static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk) { struct iwl_mvm *mvm; struct iwl_mvm_vif *mvmvif; struct ieee80211_vif *vif; mvmvif = container_of(wk, struct iwl_mvm_vif, uapsd_nonagg_detected_wk.work); vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the same thing. There are fifteen of them throughout: wireless-next/drivers/net/wireless$ grep -rn "container_of(.* ieee80211_vif" intel/iwlwifi/mvm/utils.c:794: vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); intel/iwlwifi/mvm/mac80211.c:1347: vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt76x02_mmio.c:415: vif = container_of(priv, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7615/mac.c:275: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7915/mac.c:416: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7915/mac.c:2327: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7915/debugfs.c:1026: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7921/mac.c:425: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); ti/wlcore/wlcore_i.h:502: return container_of((void *)wlvif, struct ieee80211_vif, drv_priv); realtek/rtl818x/rtl8187/dev.c:1068: container_of((void *)vif_priv, struct ieee80211_vif, drv_priv); realtek/rtl818x/rtl8180/dev.c:1293: container_of((void *)vif_priv, struct ieee80211_vif, drv_priv); realtek/rtw88/main.h:2075: return container_of(p, struct ieee80211_vif, drv_priv); realtek/rtw89/core.h:3440: return container_of(p, struct ieee80211_vif, drv_priv); ath/carl9170/carl9170.h:641: return container_of((void *)priv, struct ieee80211_vif, drv_priv); ath/wcn36xx/wcn36xx.h:329: return container_of((void *) vif_priv, struct ieee80211_vif, drv_priv); Thanks, Jaehee > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote: > On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote: > > > > Jaehee Park <jhpark1013@gmail.com> writes: > > > > > 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 function > > > 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@gmail.com> > > > > [...] > > > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > > > +{ > > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > > > +} > > > > Why the void pointer cast? Avoid casts as much possible. > > > > Hi Kalle, > > Sorry for the delay in getting back to you about why the void pointer > cast was used. > > In essence, I'm taking private data with a driver-specific pointer > and that needs to be resolved back to a generic pointer. > > The private data (drv_priv) is declared as a generic u8 array in struct > ieee80211_vif, but wvif is a more specific type. > > I wanted to also point to existing, reasonable examples such as: > static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk) > { > struct iwl_mvm *mvm; > struct iwl_mvm_vif *mvmvif; > struct ieee80211_vif *vif; > > mvmvif = container_of(wk, struct iwl_mvm_vif, > uapsd_nonagg_detected_wk.work); > vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); > > in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the > same thing. > > There are fifteen of them throughout: > wireless-next/drivers/net/wireless$ grep -rn "container_of(.* ieee80211_vif" > intel/iwlwifi/mvm/utils.c:794: vif = container_of((void *)mvmvif, > struct ieee80211_vif, drv_priv); > intel/iwlwifi/mvm/mac80211.c:1347: vif = container_of((void > *)mvmvif, struct ieee80211_vif, drv_priv); > mediatek/mt76/mt76x02_mmio.c:415: vif = > container_of(priv, struct ieee80211_vif, drv_priv); > mediatek/mt76/mt7615/mac.c:275: vif = container_of((void *)msta->vif, > struct ieee80211_vif, drv_priv); > mediatek/mt76/mt7915/mac.c:416: vif = container_of((void *)msta->vif, > struct ieee80211_vif, drv_priv); > mediatek/mt76/mt7915/mac.c:2327: vif = > container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); > mediatek/mt76/mt7915/debugfs.c:1026: vif = container_of((void > *)msta->vif, struct ieee80211_vif, drv_priv); > mediatek/mt76/mt7921/mac.c:425: vif = container_of((void *)msta->vif, > struct ieee80211_vif, drv_priv); > ti/wlcore/wlcore_i.h:502: return container_of((void *)wlvif, > struct ieee80211_vif, drv_priv); > realtek/rtl818x/rtl8187/dev.c:1068: container_of((void > *)vif_priv, struct ieee80211_vif, drv_priv); > realtek/rtl818x/rtl8180/dev.c:1293: container_of((void > *)vif_priv, struct ieee80211_vif, drv_priv); > realtek/rtw88/main.h:2075: return container_of(p, struct > ieee80211_vif, drv_priv); > realtek/rtw89/core.h:3440: return container_of(p, struct > ieee80211_vif, drv_priv); > ath/carl9170/carl9170.h:641: return container_of((void *)priv, > struct ieee80211_vif, drv_priv); > ath/wcn36xx/wcn36xx.h:329: return container_of((void *) vif_priv, > struct ieee80211_vif, drv_priv); > Sorry -- here's the last portion of the email without wrapping. wireless-next/drivers/net/wireless$ grep -rn "container_of(.* ieee80211_vif" intel/iwlwifi/mvm/utils.c:794: vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); intel/iwlwifi/mvm/mac80211.c:1347: vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt76x02_mmio.c:415: vif = container_of(priv, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7615/mac.c:275: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7915/mac.c:416: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7915/mac.c:2327: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7915/debugfs.c:1026: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); mediatek/mt76/mt7921/mac.c:425: vif = container_of((void *)msta->vif, struct ieee80211_vif, drv_priv); ti/wlcore/wlcore_i.h:502: return container_of((void *)wlvif, struct ieee80211_vif, drv_priv); realtek/rtl818x/rtl8187/dev.c:1068: container_of((void *)vif_priv, struct ieee80211_vif, drv_priv); realtek/rtl818x/rtl8180/dev.c:1293: container_of((void *)vif_priv, struct ieee80211_vif, drv_priv); realtek/rtw88/main.h:2075: return container_of(p, struct ieee80211_vif, drv_priv); realtek/rtw89/core.h:3440: return container_of(p, struct ieee80211_vif, drv_priv); ath/carl9170/carl9170.h:641: return container_of((void *)priv, struct ieee80211_vif, drv_priv); ath/wcn36xx/wcn36xx.h:329: return container_of((void *) vif_priv, struct ieee80211_vif, drv_priv); > Thanks, > Jaehee > > > > -- > > https://patchwork.kernel.org/project/linux-wireless/list/ > > > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, Apr 19, 2022 at 03:39:19PM +0200, Jérôme Pouiller wrote: > On Monday 18 April 2022 05:51:10 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 function > > 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@gmail.com> > > --- > > > > Changes from staging to wireless-next tree > > - changed macro into function and named it back to wvif_to_vif > > - fit all lines in patch to 80 columns > > - decared a reference to vif at the beginning of the functions > > > > NOTE: Jérôme is going to be testing this patch on his hardware > > Don't forget to increment the version number of your submission (option > -v of git send-email). > > > drivers/net/wireless/silabs/wfx/wfx.h | 6 +- > > drivers/net/wireless/silabs/wfx/data_rx.c | 5 +- > > drivers/net/wireless/silabs/wfx/data_tx.c | 3 +- > > drivers/net/wireless/silabs/wfx/key.c | 4 +- > > drivers/net/wireless/silabs/wfx/queue.c | 3 +- > > drivers/net/wireless/silabs/wfx/scan.c | 9 ++- > > drivers/net/wireless/silabs/wfx/sta.c | 69 ++++++++++++++--------- > > 7 files changed, 63 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h > > index 6594cc647c2f..718693a4273d 100644 > > --- a/drivers/net/wireless/silabs/wfx/wfx.h > > +++ b/drivers/net/wireless/silabs/wfx/wfx.h > > @@ -61,7 +61,6 @@ struct wfx_dev { > > > > struct wfx_vif { > > struct wfx_dev *wdev; > > - struct ieee80211_vif *vif; > > struct ieee80211_channel *channel; > > int id; > > > > @@ -91,6 +90,11 @@ struct wfx_vif { > > struct completion set_pm_mode_complete; > > }; > > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > > +{ > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > > +} > > + > > static inline struct wfx_vif *wdev_to_wvif(struct wfx_dev *wdev, int vif_id) > > { > > if (vif_id >= ARRAY_SIZE(wdev->vif)) { > > diff --git a/drivers/net/wireless/silabs/wfx/data_rx.c b/drivers/net/wireless/silabs/wfx/data_rx.c > > index a4b5ffe158e4..342b9cd0e74c 100644 > > --- a/drivers/net/wireless/silabs/wfx/data_rx.c > > +++ b/drivers/net/wireless/silabs/wfx/data_rx.c > > @@ -16,6 +16,7 @@ > > static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt) > > { > > int params, tid; > > + struct ieee80211_vif *vif = wvif_to_vif(wvif); > > When you can, try to place the longest declaration first ("reverse > Christmas tree order"). Thanks Jerome, I have a new version of the patch I'm going to send today with this edit. > > [...] > > diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c > > index 3297d73c327a..97fcbad23c94 100644 > > --- a/drivers/net/wireless/silabs/wfx/sta.c > > +++ b/drivers/net/wireless/silabs/wfx/sta.c > [...] > > @@ -152,19 +153,28 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps) > > { > > struct ieee80211_channel *chan0 = NULL, *chan1 = NULL; > > struct ieee80211_conf *conf = &wvif->wdev->hw->conf; > > + struct ieee80211_vif *vif = wvif_to_vif(wvif); > > > > - WARN(!wvif->vif->bss_conf.assoc && enable_ps, > > + WARN(!vif->bss_conf.assoc && enable_ps, > > "enable_ps is reliable only if associated"); > > - if (wdev_to_wvif(wvif->wdev, 0)) > > - chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan; > > - if (wdev_to_wvif(wvif->wdev, 1)) > > - chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan; > > - if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { > > + if (wdev_to_wvif(wvif->wdev, 0)) { > > + struct wfx_vif *wvif_ch0 = wdev_to_wvif(wvif->wdev, 0); > > + struct ieee80211_vif *vif_ch0 = wvif_to_vif(wvif_ch0); > > + > > + chan0 = vif_ch0->bss_conf.chandef.chan; > > + } > > + if (wdev_to_wvif(wvif->wdev, 1)) { > > + struct wfx_vif *wvif_ch1 = wdev_to_wvif(wvif->wdev, 1); > > + struct ieee80211_vif *vif_ch1 = wvif_to_vif(wvif_ch1); > > + > > + chan1 = vif_ch1->bss_conf.chandef.chan; > > + } > > I think this code could be simplified into: > > if (wvif->wdev->vif[1]) > chan1 = wvif->wdev->vif[1]->bss_conf.chandef.chan; > > (If you choose this way, I suggest to place this change in a separate > patch) > Ok I'll send your suggested edit in a separate patch after this patch gets through. > [...] > > -- > Jérôme Pouiller > >
On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote: > On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote: > > > > Jaehee Park <jhpark1013@gmail.com> writes: > > > > > 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 function > > > 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@gmail.com> > > > > [...] > > > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > > > +{ > > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > > > +} > > > > Why the void pointer cast? Avoid casts as much possible. > > > > Hi Kalle, > > Sorry for the delay in getting back to you about why the void pointer > cast was used. > > In essence, I'm taking private data with a driver-specific pointer > and that needs to be resolved back to a generic pointer. > > The private data (drv_priv) is declared as a generic u8 array in struct > ieee80211_vif, but wvif is a more specific type. > > I wanted to also point to existing, reasonable examples such as: > static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk) > { > struct iwl_mvm *mvm; > struct iwl_mvm_vif *mvmvif; > struct ieee80211_vif *vif; > > mvmvif = container_of(wk, struct iwl_mvm_vif, > uapsd_nonagg_detected_wk.work); > vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); > > in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the > same thing. > > There are fifteen of them throughout: The cast is fine, but this email is frustrating. It sounds like you are saying that you copied it from other code and that's not a good answer... :/ It's easiest if you just copy and paste the build error and we can figure out why the cast is need for our selves... drivers/net/wireless/silabs/wfx/data_rx.c: In function ‘wvif_to_vif’: ./include/linux/build_bug.h:78:41: error: static assertion failed: "pointer type mismatch in container_of()" 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~~~~~~~~~~~ ./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’ 77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr) | ^~~~~~~~~~~~~~~ ./include/linux/container_of.h:19:9: note: in expansion of macro ‘static_assert’ 19 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \ | ^~~~~~~~~~~~~ drivers/net/wireless/silabs/wfx/data_rx.c:20:16: note: in expansion of macro ‘container_of’ 20 | return container_of(wvif, struct ieee80211_vif, drv_priv); | ^~~~~~~~~~~~ regards, dan carpenter
Hi Dan, On Wed, 4 May 2022 12:33:48 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Mon, May 02, 2022 at 02:10:07PM -0400, Jaehee wrote: > > On Wed, Apr 20, 2022 at 7:58 AM Kalle Valo <kvalo@kernel.org> wrote: > > > > > > Jaehee Park <jhpark1013@gmail.com> writes: > > > > > > > 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 function > > > > 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@gmail.com> > > > > > > [...] > > > > > > > +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) > > > > +{ > > > > + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); > > > > +} > > > > > > Why the void pointer cast? Avoid casts as much possible. > > > > > > > Hi Kalle, > > > > Sorry for the delay in getting back to you about why the void pointer > > cast was used. > > > > In essence, I'm taking private data with a driver-specific pointer > > and that needs to be resolved back to a generic pointer. > > > > The private data (drv_priv) is declared as a generic u8 array in struct > > ieee80211_vif, but wvif is a more specific type. > > > > I wanted to also point to existing, reasonable examples such as: > > static void iwl_mvm_tcm_uapsd_nonagg_detected_wk(struct work_struct *wk) > > { > > struct iwl_mvm *mvm; > > struct iwl_mvm_vif *mvmvif; > > struct ieee80211_vif *vif; > > > > mvmvif = container_of(wk, struct iwl_mvm_vif, > > uapsd_nonagg_detected_wk.work); > > vif = container_of((void *)mvmvif, struct ieee80211_vif, drv_priv); > > > > in drivers/net/wireless$ less intel/iwlwifi/mvm/utils.c, which does the > > same thing. > > > > There are fifteen of them throughout: > > The cast is fine, but this email is frustrating. > > It sounds like you are saying that you copied it from other code and > that's not a good answer... :/ It's easiest if you just copy and paste > the build error and we can figure out why the cast is need for our > selves... ...my bad, then. I suggested to Jaehee she would *also* point out that there are already a pile of usages (which I grepped for myself, by the way). And that it's *obvious* that container_of() would trigger warnings otherwise. Well, obvious just for me, it seems.
On Wed, May 04, 2022 at 01:50:59PM +0200, Stefano Brivio wrote: > And that it's *obvious* that container_of() would trigger warnings > otherwise. Well, obvious just for me, it seems. :P Apparently it wasn't obvious to Kalle and me. My guess is that you saw the build error. Either that or Kalle and I are geezers who haven't looked at container_of() since before the BUILD_BUG_ON() was added five years ago. regards, dan carpenter
Dan Carpenter <dan.carpenter@oracle.com> writes: > On Wed, May 04, 2022 at 01:50:59PM +0200, Stefano Brivio wrote: >> And that it's *obvious* that container_of() would trigger warnings >> otherwise. Well, obvious just for me, it seems. > > :P > > Apparently it wasn't obvious to Kalle and me. My guess is that you saw > the build error. Either that or Kalle and I are geezers who haven't > looked at container_of() since before the BUILD_BUG_ON() was added five > years ago. Exactly, I also had to duplicate the error myself before I was able to understand the issue. So I'm officially a geezer now :D
On Wed, May 04, 2022 at 07:05:57PM +0300, Kalle Valo wrote: > Dan Carpenter <dan.carpenter@oracle.com> writes: > > > On Wed, May 04, 2022 at 01:50:59PM +0200, Stefano Brivio wrote: > >> And that it's *obvious* that container_of() would trigger warnings > >> otherwise. Well, obvious just for me, it seems. > > > > :P > > > > Apparently it wasn't obvious to Kalle and me. My guess is that you saw > > the build error. Either that or Kalle and I are geezers who haven't > > looked at container_of() since before the BUILD_BUG_ON() was added five > > years ago. > > Exactly, I also had to duplicate the error myself before I was able to > understand the issue. So I'm officially a geezer now :D > Hi Dan and Kalle, thanks for your messages! Understood -- I'll copy the error for reference next time. Sorry you had to recreate the build error yourself! > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h index 6594cc647c2f..718693a4273d 100644 --- a/drivers/net/wireless/silabs/wfx/wfx.h +++ b/drivers/net/wireless/silabs/wfx/wfx.h @@ -61,7 +61,6 @@ struct wfx_dev { struct wfx_vif { struct wfx_dev *wdev; - struct ieee80211_vif *vif; struct ieee80211_channel *channel; int id; @@ -91,6 +90,11 @@ struct wfx_vif { struct completion set_pm_mode_complete; }; +static inline struct ieee80211_vif *wvif_to_vif(struct wfx_vif *wvif) +{ + return container_of((void *)wvif, struct ieee80211_vif, drv_priv); +} + static inline struct wfx_vif *wdev_to_wvif(struct wfx_dev *wdev, int vif_id) { if (vif_id >= ARRAY_SIZE(wdev->vif)) { diff --git a/drivers/net/wireless/silabs/wfx/data_rx.c b/drivers/net/wireless/silabs/wfx/data_rx.c index a4b5ffe158e4..342b9cd0e74c 100644 --- a/drivers/net/wireless/silabs/wfx/data_rx.c +++ b/drivers/net/wireless/silabs/wfx/data_rx.c @@ -16,6 +16,7 @@ static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt) { int params, tid; + struct ieee80211_vif *vif = wvif_to_vif(wvif); if (wfx_api_older_than(wvif->wdev, 3, 6)) return; @@ -24,12 +25,12 @@ static void wfx_rx_handle_ba(struct wfx_vif *wvif, struct ieee80211_mgmt *mgmt) case WLAN_ACTION_ADDBA_REQ: params = le16_to_cpu(mgmt->u.action.u.addba_req.capab); tid = (params & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2; - ieee80211_start_rx_ba_session_offl(wvif->vif, mgmt->sa, tid); + ieee80211_start_rx_ba_session_offl(vif, mgmt->sa, tid); break; case WLAN_ACTION_DELBA: params = le16_to_cpu(mgmt->u.action.u.delba.params); tid = (params & IEEE80211_DELBA_PARAM_TID_MASK) >> 12; - ieee80211_stop_rx_ba_session_offl(wvif->vif, mgmt->sa, tid); + ieee80211_stop_rx_ba_session_offl(vif, mgmt->sa, tid); break; } } diff --git a/drivers/net/wireless/silabs/wfx/data_tx.c b/drivers/net/wireless/silabs/wfx/data_tx.c index e07381b2ff4d..db99142e8e8f 100644 --- a/drivers/net/wireless/silabs/wfx/data_tx.c +++ b/drivers/net/wireless/silabs/wfx/data_tx.c @@ -213,10 +213,11 @@ static u8 wfx_tx_get_link_id(struct wfx_vif *wvif, struct ieee80211_sta *sta, { struct wfx_sta_priv *sta_priv = sta ? (struct wfx_sta_priv *)&sta->drv_priv : NULL; const u8 *da = ieee80211_get_DA(hdr); + struct ieee80211_vif *vif = wvif_to_vif(wvif); if (sta_priv && sta_priv->link_id) return sta_priv->link_id; - if (wvif->vif->type != NL80211_IFTYPE_AP) + if (vif->type != NL80211_IFTYPE_AP) return 0; if (is_multicast_ether_addr(da)) return 0; diff --git a/drivers/net/wireless/silabs/wfx/key.c b/drivers/net/wireless/silabs/wfx/key.c index 8f23e8d42bd4..196d64ef68f3 100644 --- a/drivers/net/wireless/silabs/wfx/key.c +++ b/drivers/net/wireless/silabs/wfx/key.c @@ -156,6 +156,7 @@ static int wfx_add_key(struct wfx_vif *wvif, struct ieee80211_sta *sta, struct wfx_dev *wdev = wvif->wdev; int idx = wfx_alloc_key(wvif->wdev); bool pairwise = key->flags & IEEE80211_KEY_FLAG_PAIRWISE; + struct ieee80211_vif *vif = wvif_to_vif(wvif); WARN(key->flags & IEEE80211_KEY_FLAG_PAIRWISE && !sta, "inconsistent data"); ieee80211_get_key_rx_seq(key, 0, &seq); @@ -174,7 +175,7 @@ static int wfx_add_key(struct wfx_vif *wvif, struct ieee80211_sta *sta, k.type = fill_tkip_pair(&k.key.tkip_pairwise_key, key, sta->addr); else k.type = fill_tkip_group(&k.key.tkip_group_key, key, &seq, - wvif->vif->type); + vif->type); } else if (key->cipher == WLAN_CIPHER_SUITE_CCMP) { if (pairwise) k.type = fill_ccmp_pair(&k.key.aes_pairwise_key, key, sta->addr); @@ -224,4 +225,3 @@ int wfx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, struct ieee80211_ mutex_unlock(&wvif->wdev->conf_mutex); return ret; } - diff --git a/drivers/net/wireless/silabs/wfx/queue.c b/drivers/net/wireless/silabs/wfx/queue.c index 729825230db2..fa1837a00a74 100644 --- a/drivers/net/wireless/silabs/wfx/queue.c +++ b/drivers/net/wireless/silabs/wfx/queue.c @@ -206,8 +206,9 @@ unsigned int wfx_pending_get_pkt_us_delay(struct wfx_dev *wdev, struct sk_buff * bool wfx_tx_queues_has_cab(struct wfx_vif *wvif) { int i; + struct ieee80211_vif *vif = wvif_to_vif(wvif); - if (wvif->vif->type != NL80211_IFTYPE_AP) + if (vif->type != NL80211_IFTYPE_AP) return false; for (i = 0; i < IEEE80211_NUM_ACS; ++i) /* Note: since only AP can have mcast frames in queue and only one vif can be AP, diff --git a/drivers/net/wireless/silabs/wfx/scan.c b/drivers/net/wireless/silabs/wfx/scan.c index 7f34f0d322f9..d2dd9829351a 100644 --- a/drivers/net/wireless/silabs/wfx/scan.c +++ b/drivers/net/wireless/silabs/wfx/scan.c @@ -24,8 +24,10 @@ static void wfx_ieee80211_scan_completed_compat(struct ieee80211_hw *hw, bool ab static int update_probe_tmpl(struct wfx_vif *wvif, struct cfg80211_scan_request *req) { struct sk_buff *skb; + struct ieee80211_vif *vif = wvif_to_vif(wvif); - skb = ieee80211_probereq_get(wvif->wdev->hw, wvif->vif->addr, NULL, 0, req->ie_len); + skb = ieee80211_probereq_get(wvif->wdev->hw, vif->addr, NULL, 0, + req->ie_len); if (!skb) return -ENOMEM; @@ -39,6 +41,7 @@ static int send_scan_req(struct wfx_vif *wvif, struct cfg80211_scan_request *req { int i, ret; struct ieee80211_channel *ch_start, *ch_cur; + struct ieee80211_vif *vif = wvif_to_vif(wvif); for (i = start_idx; i < req->n_channels; i++) { ch_start = req->channels[start_idx]; @@ -75,8 +78,8 @@ static int send_scan_req(struct wfx_vif *wvif, struct cfg80211_scan_request *req } else { ret = wvif->scan_nb_chan_done; } - if (req->channels[start_idx]->max_power != wvif->vif->bss_conf.txpower) - wfx_hif_set_output_power(wvif, wvif->vif->bss_conf.txpower); + if (req->channels[start_idx]->max_power != vif->bss_conf.txpower) + wfx_hif_set_output_power(wvif, vif->bss_conf.txpower); wfx_tx_unlock(wvif->wdev); return ret; } diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c index 3297d73c327a..97fcbad23c94 100644 --- a/drivers/net/wireless/silabs/wfx/sta.c +++ b/drivers/net/wireless/silabs/wfx/sta.c @@ -101,6 +101,7 @@ void wfx_configure_filter(struct ieee80211_hw *hw, unsigned int changed_flags, struct wfx_vif *wvif = NULL; struct wfx_dev *wdev = hw->priv; bool filter_bssid, filter_prbreq, filter_beacon; + struct ieee80211_vif *vif = wvif_to_vif(wvif); /* Notes: * - Probe responses (FIF_BCN_PRBRESP_PROMISC) are never filtered @@ -132,7 +133,7 @@ void wfx_configure_filter(struct ieee80211_hw *hw, unsigned int changed_flags, filter_bssid = true; /* In AP mode, chip can reply to probe request itself */ - if (*total_flags & FIF_PROBE_REQ && wvif->vif->type == NL80211_IFTYPE_AP) { + if (*total_flags & FIF_PROBE_REQ && vif->type == NL80211_IFTYPE_AP) { dev_dbg(wdev->dev, "do not forward probe request in AP mode\n"); *total_flags &= ~FIF_PROBE_REQ; } @@ -152,19 +153,28 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps) { struct ieee80211_channel *chan0 = NULL, *chan1 = NULL; struct ieee80211_conf *conf = &wvif->wdev->hw->conf; + struct ieee80211_vif *vif = wvif_to_vif(wvif); - WARN(!wvif->vif->bss_conf.assoc && enable_ps, + WARN(!vif->bss_conf.assoc && enable_ps, "enable_ps is reliable only if associated"); - if (wdev_to_wvif(wvif->wdev, 0)) - chan0 = wdev_to_wvif(wvif->wdev, 0)->vif->bss_conf.chandef.chan; - if (wdev_to_wvif(wvif->wdev, 1)) - chan1 = wdev_to_wvif(wvif->wdev, 1)->vif->bss_conf.chandef.chan; - if (chan0 && chan1 && wvif->vif->type != NL80211_IFTYPE_AP) { + if (wdev_to_wvif(wvif->wdev, 0)) { + struct wfx_vif *wvif_ch0 = wdev_to_wvif(wvif->wdev, 0); + struct ieee80211_vif *vif_ch0 = wvif_to_vif(wvif_ch0); + + chan0 = vif_ch0->bss_conf.chandef.chan; + } + if (wdev_to_wvif(wvif->wdev, 1)) { + struct wfx_vif *wvif_ch1 = wdev_to_wvif(wvif->wdev, 1); + struct ieee80211_vif *vif_ch1 = wvif_to_vif(wvif_ch1); + + chan1 = vif_ch1->bss_conf.chandef.chan; + } + if (chan0 && chan1 && vif->type != NL80211_IFTYPE_AP) { if (chan0->hw_value == chan1->hw_value) { /* It is useless to enable PS if channels are the same. */ if (enable_ps) *enable_ps = false; - if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps) + if (vif->bss_conf.assoc && vif->bss_conf.ps) dev_info(wvif->wdev->dev, "ignoring requested PS mode"); return -1; } @@ -177,8 +187,8 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool *enable_ps) return 30; } if (enable_ps) - *enable_ps = wvif->vif->bss_conf.ps; - if (wvif->vif->bss_conf.assoc && wvif->vif->bss_conf.ps) + *enable_ps = vif->bss_conf.ps; + if (vif->bss_conf.assoc && vif->bss_conf.ps) return conf->dynamic_ps_timeout; else return -1; @@ -188,8 +198,9 @@ int wfx_update_pm(struct wfx_vif *wvif) { int ps_timeout; bool ps; + struct ieee80211_vif *vif = wvif_to_vif(wvif); - if (!wvif->vif->bss_conf.assoc) + if (!vif->bss_conf.assoc) return 0; ps_timeout = wfx_get_ps_timeout(wvif, &ps); if (!ps) @@ -215,7 +226,8 @@ int wfx_conf_tx(struct ieee80211_hw *hw, struct ieee80211_vif *vif, mutex_lock(&wdev->conf_mutex); assign_bit(queue, &wvif->uapsd_mask, params->uapsd); wfx_hif_set_edca_queue_params(wvif, queue, params); - if (wvif->vif->type == NL80211_IFTYPE_STATION && old_uapsd != wvif->uapsd_mask) { + if (vif->type == NL80211_IFTYPE_STATION && + old_uapsd != wvif->uapsd_mask) { wfx_hif_set_uapsd_info(wvif, wvif->uapsd_mask); wfx_update_pm(wvif); } @@ -240,22 +252,24 @@ void wfx_event_report_rssi(struct wfx_vif *wvif, u8 raw_rcpi_rssi) */ int rcpi_rssi; int cqm_evt; + struct ieee80211_vif *vif = wvif_to_vif(wvif); rcpi_rssi = raw_rcpi_rssi / 2 - 110; - if (rcpi_rssi <= wvif->vif->bss_conf.cqm_rssi_thold) + if (rcpi_rssi <= vif->bss_conf.cqm_rssi_thold) cqm_evt = NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW; else cqm_evt = NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH; - ieee80211_cqm_rssi_notify(wvif->vif, cqm_evt, rcpi_rssi, GFP_KERNEL); + ieee80211_cqm_rssi_notify(vif, cqm_evt, rcpi_rssi, GFP_KERNEL); } static void wfx_beacon_loss_work(struct work_struct *work) { struct wfx_vif *wvif = container_of(to_delayed_work(work), struct wfx_vif, beacon_loss_work); - struct ieee80211_bss_conf *bss_conf = &wvif->vif->bss_conf; + struct ieee80211_vif *vif = wvif_to_vif(wvif); + struct ieee80211_bss_conf *bss_conf = &vif->bss_conf; - ieee80211_beacon_loss(wvif->vif); + ieee80211_beacon_loss(vif); schedule_delayed_work(to_delayed_work(work), msecs_to_jiffies(bss_conf->beacon_int)); } @@ -322,14 +336,15 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ie static int wfx_upload_ap_templates(struct wfx_vif *wvif) { struct sk_buff *skb; + struct ieee80211_vif *vif = wvif_to_vif(wvif); - skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif); + skb = ieee80211_beacon_get(wvif->wdev->hw, vif); if (!skb) return -ENOMEM; wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_BCN, API_RATE_INDEX_B_1MBPS); dev_kfree_skb(skb); - skb = ieee80211_proberesp_get(wvif->wdev->hw, wvif->vif); + skb = ieee80211_proberesp_get(wvif->wdev->hw, vif); if (!skb) return -ENOMEM; wfx_hif_set_template_frame(wvif, skb, HIF_TMPLT_PRBRES, API_RATE_INDEX_B_1MBPS); @@ -339,7 +354,8 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif) static void wfx_set_mfp_ap(struct wfx_vif *wvif) { - struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif); + struct ieee80211_vif *vif = wvif_to_vif(wvif); + struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, vif); const int ieoffset = offsetof(struct ieee80211_mgmt, u.beacon.variable); const u16 *ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset, skb->len - ieoffset); @@ -389,7 +405,8 @@ void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif) static void wfx_join(struct wfx_vif *wvif) { int ret; - struct ieee80211_bss_conf *conf = &wvif->vif->bss_conf; + struct ieee80211_vif *vif = wvif_to_vif(wvif); + struct ieee80211_bss_conf *conf = &vif->bss_conf; struct cfg80211_bss *bss = NULL; u8 ssid[IEEE80211_MAX_SSID_LEN]; const u8 *ssidie = NULL; @@ -420,7 +437,7 @@ static void wfx_join(struct wfx_vif *wvif) wvif->join_in_progress = true; ret = wfx_hif_join(wvif, conf, wvif->channel, ssid, ssidlen); if (ret) { - ieee80211_connection_loss(wvif->vif); + ieee80211_connection_loss(vif); wfx_reset(wvif); } else { /* Due to beacon filtering it is possible that the AP's beacon is not known for the @@ -437,10 +454,11 @@ static void wfx_join_finalize(struct wfx_vif *wvif, struct ieee80211_bss_conf *i struct ieee80211_sta *sta = NULL; int ampdu_density = 0; bool greenfield = false; + struct ieee80211_vif *vif = wvif_to_vif(wvif); rcu_read_lock(); /* protect sta */ if (info->bssid && !info->ibss_joined) - sta = ieee80211_find_sta(wvif->vif, info->bssid); + sta = ieee80211_find_sta(vif, info->bssid); if (sta && sta->deflink.ht_cap.ht_supported) ampdu_density = sta->deflink.ht_cap.ampdu_density; if (sta && sta->deflink.ht_cap.ht_supported && @@ -564,8 +582,10 @@ static int wfx_update_tim(struct wfx_vif *wvif) struct sk_buff *skb; u16 tim_offset, tim_length; u8 *tim_ptr; + struct ieee80211_vif *vif = wvif_to_vif(wvif); - skb = ieee80211_beacon_get_tim(wvif->wdev->hw, wvif->vif, &tim_offset, &tim_length); + skb = ieee80211_beacon_get_tim(wvif->wdev->hw, vif, &tim_offset, + &tim_length); if (!skb) return -ENOENT; tim_ptr = skb->data + tim_offset; @@ -707,8 +727,6 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) return -EOPNOTSUPP; } - /* FIXME: prefer use of container_of() to get vif */ - wvif->vif = vif; wvif->wdev = wdev; wvif->link_id_map = 1; /* link-id 0 is reserved for multicast */ @@ -767,7 +785,6 @@ void wfx_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif) cancel_delayed_work_sync(&wvif->beacon_loss_work); wdev->vif[wvif->id] = NULL; - wvif->vif = NULL; mutex_unlock(&wdev->conf_mutex);
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 function 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@gmail.com> --- Changes from staging to wireless-next tree - changed macro into function and named it back to wvif_to_vif - fit all lines in patch to 80 columns - decared a reference to vif at the beginning of the functions NOTE: Jérôme is going to be testing this patch on his hardware drivers/net/wireless/silabs/wfx/wfx.h | 6 +- drivers/net/wireless/silabs/wfx/data_rx.c | 5 +- drivers/net/wireless/silabs/wfx/data_tx.c | 3 +- drivers/net/wireless/silabs/wfx/key.c | 4 +- drivers/net/wireless/silabs/wfx/queue.c | 3 +- drivers/net/wireless/silabs/wfx/scan.c | 9 ++- drivers/net/wireless/silabs/wfx/sta.c | 69 ++++++++++++++--------- 7 files changed, 63 insertions(+), 36 deletions(-)