| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 May 2022 14:18:53 -0400
From: Jaehee Park <jhpark1013@...il.com>
To: Jérôme Pouiller <jerome.pouiller@...abs.com>
Cc: Kalle Valo <kvalo@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
outreachy@...ts.linux.dev, Stefano Brivio <sbrivio@...hat.com>
Subject: Re: [PATCH] wfx: use container_of() to get vif
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@...il.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
>
>
Powered by blists - more mailing lists