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

["Jérôme Pouiller" <jerome.pouiller@silabs.com>]

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)

[...]

-- 
Jérôme Pouiller