[v1] mwifiex: two fixes and cleanup

[PATCH 11/12] wifi: mwifiex: move common settings out of switch/case

Posted by Sascha Hauer 1 year, 3 months ago

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;
+	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);

-- 
2.39.2

Re: [PATCH 11/12] wifi: mwifiex: move common settings out of switch/case

Posted by Francesco Dolcini 1 year, 3 months ago

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?

Re: [PATCH 11/12] wifi: mwifiex: move common settings out of switch/case

Posted by Sascha Hauer 1 year, 3 months ago

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 11/12] wifi: mwifiex: move common settings out of switch/case

Posted by Francesco Dolcini 1 year, 3 months ago

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