| Message ID | 20240826-mwifiex-cleanup-1-v1-11-56e6f8e056ec@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mwifiex: two fixes and cleanup | expand |
On Mon, Aug 26, 2024 at 01:01:32PM +0200, Sascha Hauer wrote: > In mwifiex_add_virtual_intf() several settings done in a switch/case > are the same in all cases. Move them out of the switch/case to > deduplicate the code. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > index 8746943c17788..2ce54a3fc32f8 100644 > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > @@ -3005,7 +3005,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > return ERR_PTR(-EFAULT); > } > > - priv->wdev.wiphy = wiphy; > priv->wdev.iftype = NL80211_IFTYPE_STATION; > > if (type == NL80211_IFTYPE_UNSPECIFIED) > @@ -3014,8 +3013,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > priv->bss_mode = type; > > priv->bss_type = MWIFIEX_BSS_TYPE_STA; > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; > - priv->bss_priority = 0; > priv->bss_role = MWIFIEX_BSS_ROLE_STA; > > break; > @@ -3035,14 +3032,10 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > return ERR_PTR(-EFAULT); > } > > - priv->wdev.wiphy = wiphy; > priv->wdev.iftype = NL80211_IFTYPE_AP; > > priv->bss_type = MWIFIEX_BSS_TYPE_UAP; > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; > - priv->bss_priority = 0; > priv->bss_role = MWIFIEX_BSS_ROLE_UAP; > - priv->bss_started = 0; > priv->bss_mode = type; > > break; > @@ -3062,7 +3055,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > return ERR_PTR(-EFAULT); > } > > - priv->wdev.wiphy = wiphy; > /* At start-up, wpa_supplicant tries to change the interface > * to NL80211_IFTYPE_STATION if it is not managed mode. > */ > @@ -3075,10 +3067,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > */ > priv->bss_type = MWIFIEX_BSS_TYPE_P2P; > > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; > - priv->bss_priority = 0; > priv->bss_role = MWIFIEX_BSS_ROLE_STA; > - priv->bss_started = 0; > > if (mwifiex_cfg80211_init_p2p_client(priv)) { > memset(&priv->wdev, 0, sizeof(priv->wdev)); > @@ -3092,6 +3081,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > return ERR_PTR(-EINVAL); > } > > + priv->wdev.wiphy = wiphy; > + priv->bss_priority = 0; > + priv->bss_started = 0; This was not set before in all the 3 cases. Irrelevant? Worth checking and/or mentioning in the commit message?
On Mon, Sep 09, 2024 at 07:09:19PM +0200, Francesco Dolcini wrote: > On Mon, Aug 26, 2024 at 01:01:32PM +0200, Sascha Hauer wrote: > > In mwifiex_add_virtual_intf() several settings done in a switch/case > > are the same in all cases. Move them out of the switch/case to > > deduplicate the code. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 16 +++++----------- > > 1 file changed, 5 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > index 8746943c17788..2ce54a3fc32f8 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c > > @@ -3005,7 +3005,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > > return ERR_PTR(-EFAULT); > > } > > > > - priv->wdev.wiphy = wiphy; > > priv->wdev.iftype = NL80211_IFTYPE_STATION; > > > > if (type == NL80211_IFTYPE_UNSPECIFIED) > > @@ -3014,8 +3013,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > > priv->bss_mode = type; > > > > priv->bss_type = MWIFIEX_BSS_TYPE_STA; > > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; > > - priv->bss_priority = 0; > > priv->bss_role = MWIFIEX_BSS_ROLE_STA; > > > > break; > > @@ -3035,14 +3032,10 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > > return ERR_PTR(-EFAULT); > > } > > > > - priv->wdev.wiphy = wiphy; > > priv->wdev.iftype = NL80211_IFTYPE_AP; > > > > priv->bss_type = MWIFIEX_BSS_TYPE_UAP; > > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; > > - priv->bss_priority = 0; > > priv->bss_role = MWIFIEX_BSS_ROLE_UAP; > > - priv->bss_started = 0; > > priv->bss_mode = type; > > > > break; > > @@ -3062,7 +3055,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > > return ERR_PTR(-EFAULT); > > } > > > > - priv->wdev.wiphy = wiphy; > > /* At start-up, wpa_supplicant tries to change the interface > > * to NL80211_IFTYPE_STATION if it is not managed mode. > > */ > > @@ -3075,10 +3067,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > > */ > > priv->bss_type = MWIFIEX_BSS_TYPE_P2P; > > > > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; > > - priv->bss_priority = 0; > > priv->bss_role = MWIFIEX_BSS_ROLE_STA; > > - priv->bss_started = 0; > > > > if (mwifiex_cfg80211_init_p2p_client(priv)) { > > memset(&priv->wdev, 0, sizeof(priv->wdev)); > > @@ -3092,6 +3081,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, > > return ERR_PTR(-EINVAL); > > } > > > > + priv->wdev.wiphy = wiphy; > > + priv->bss_priority = 0; > > + priv->bss_started = 0; > > This was not set before in all the 3 cases. Irrelevant? Worth checking and/or > mentioning in the commit message? bss_started is only used in AP mode, its value is irrelevant in station or adhoc mode. I'll add that to the commit message. Sascha
Il 9 settembre 2024 22:21:41 CEST, Sascha Hauer <s.hauer@pengutronix.de> ha scritto: >On Mon, Sep 09, 2024 at 07:09:19PM +0200, Francesco Dolcini wrote: >> On Mon, Aug 26, 2024 at 01:01:32PM +0200, Sascha Hauer wrote: >> > In mwifiex_add_virtual_intf() several settings done in a switch/case >> > are the same in all cases. Move them out of the switch/case to >> > deduplicate the code. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > --- >> > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 16 +++++----------- >> > 1 file changed, 5 insertions(+), 11 deletions(-) >> > >> > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> > index 8746943c17788..2ce54a3fc32f8 100644 >> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> > @@ -3005,7 +3005,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, >> > return ERR_PTR(-EFAULT); >> > } >> > >> > - priv->wdev.wiphy = wiphy; >> > priv->wdev.iftype = NL80211_IFTYPE_STATION; >> > >> > if (type == NL80211_IFTYPE_UNSPECIFIED) >> > @@ -3014,8 +3013,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, >> > priv->bss_mode = type; >> > >> > priv->bss_type = MWIFIEX_BSS_TYPE_STA; >> > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; >> > - priv->bss_priority = 0; >> > priv->bss_role = MWIFIEX_BSS_ROLE_STA; >> > >> > break; >> > @@ -3035,14 +3032,10 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, >> > return ERR_PTR(-EFAULT); >> > } >> > >> > - priv->wdev.wiphy = wiphy; >> > priv->wdev.iftype = NL80211_IFTYPE_AP; >> > >> > priv->bss_type = MWIFIEX_BSS_TYPE_UAP; >> > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; >> > - priv->bss_priority = 0; >> > priv->bss_role = MWIFIEX_BSS_ROLE_UAP; >> > - priv->bss_started = 0; >> > priv->bss_mode = type; >> > >> > break; >> > @@ -3062,7 +3055,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, >> > return ERR_PTR(-EFAULT); >> > } >> > >> > - priv->wdev.wiphy = wiphy; >> > /* At start-up, wpa_supplicant tries to change the interface >> > * to NL80211_IFTYPE_STATION if it is not managed mode. >> > */ >> > @@ -3075,10 +3067,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, >> > */ >> > priv->bss_type = MWIFIEX_BSS_TYPE_P2P; >> > >> > - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; >> > - priv->bss_priority = 0; >> > priv->bss_role = MWIFIEX_BSS_ROLE_STA; >> > - priv->bss_started = 0; >> > >> > if (mwifiex_cfg80211_init_p2p_client(priv)) { >> > memset(&priv->wdev, 0, sizeof(priv->wdev)); >> > @@ -3092,6 +3081,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, >> > return ERR_PTR(-EINVAL); >> > } >> > >> > + priv->wdev.wiphy = wiphy; >> > + priv->bss_priority = 0; >> > + priv->bss_started = 0; >> >> This was not set before in all the 3 cases. Irrelevant? Worth checking and/or >> mentioning in the commit message? > >bss_started is only used in AP mode, its value is irrelevant in station >or adhoc mode. I'll add that to the commit message. ack. With this clarified in the commit message adds my reviewed-by
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index 8746943c17788..2ce54a3fc32f8 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -3005,7 +3005,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, return ERR_PTR(-EFAULT); } - priv->wdev.wiphy = wiphy; priv->wdev.iftype = NL80211_IFTYPE_STATION; if (type == NL80211_IFTYPE_UNSPECIFIED) @@ -3014,8 +3013,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, priv->bss_mode = type; priv->bss_type = MWIFIEX_BSS_TYPE_STA; - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; - priv->bss_priority = 0; priv->bss_role = MWIFIEX_BSS_ROLE_STA; break; @@ -3035,14 +3032,10 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, return ERR_PTR(-EFAULT); } - priv->wdev.wiphy = wiphy; priv->wdev.iftype = NL80211_IFTYPE_AP; priv->bss_type = MWIFIEX_BSS_TYPE_UAP; - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; - priv->bss_priority = 0; priv->bss_role = MWIFIEX_BSS_ROLE_UAP; - priv->bss_started = 0; priv->bss_mode = type; break; @@ -3062,7 +3055,6 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, return ERR_PTR(-EFAULT); } - priv->wdev.wiphy = wiphy; /* At start-up, wpa_supplicant tries to change the interface * to NL80211_IFTYPE_STATION if it is not managed mode. */ @@ -3075,10 +3067,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, */ priv->bss_type = MWIFIEX_BSS_TYPE_P2P; - priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; - priv->bss_priority = 0; priv->bss_role = MWIFIEX_BSS_ROLE_STA; - priv->bss_started = 0; if (mwifiex_cfg80211_init_p2p_client(priv)) { memset(&priv->wdev, 0, sizeof(priv->wdev)); @@ -3092,6 +3081,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, return ERR_PTR(-EINVAL); } + priv->wdev.wiphy = wiphy; + priv->bss_priority = 0; + priv->bss_started = 0; + priv->frame_type = MWIFIEX_DATA_FRAME_TYPE_ETH_II; + dev = alloc_netdev_mqs(sizeof(struct mwifiex_private *), name, name_assign_type, ether_setup, IEEE80211_NUM_ACS, 1);
In mwifiex_add_virtual_intf() several settings done in a switch/case are the same in all cases. Move them out of the switch/case to deduplicate the code. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)