Re: [PATCH 11/31] wifi: mwifiex: use priv index as bss_num

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Aug 20, 2024 at 01:55:36PM +0200, Sascha Hauer wrote:
> Instead of looking up an unused bss_num each time we add a virtual
> interface, associate a fixed bss_num to each priv and for simplicity
> just use the array index.
> 
> With bss_num unique to each priv mwifiex_get_priv_by_id() doesn't need
> the bss_type argument anymore, so it's removed.

I misunderstood the driver here. I thought bss_num specifies the
instance and bss_type specifies the type of this instance. It's the
other way round: bss_num is the nth instance of type bss_type. Also
the device doesn't have 16 instances of configurable type, instead
it only has 1 station mode instance. Hence, when
bss_type == MWIFIEX_BSS_TYPE_STA every other bss_num than 0 is invalid.
Likewise there are also only three instances of type
MWIFIEX_BSS_TYPE_UAP available.

I made some assumptions on this misunderstanding throughout this series,
so it needs rework.

It would be really great to have some documentation about these devices.

Sascha

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c  | 11 ++---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c    |  6 +--
>  drivers/net/wireless/marvell/mwifiex/main.c      |  1 +
>  drivers/net/wireless/marvell/mwifiex/main.h      | 54 ++++--------------------
>  drivers/net/wireless/marvell/mwifiex/sta_event.c |  3 +-
>  drivers/net/wireless/marvell/mwifiex/txrx.c      |  9 ++--
>  6 files changed, 19 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 784f342a9bf23..d5a2c8f726b9e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -952,8 +952,6 @@ mwifiex_init_new_priv_params(struct mwifiex_private *priv,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	priv->bss_num = mwifiex_get_unused_bss_num(adapter, priv->bss_type);
> -
>  	spin_lock_irqsave(&adapter->main_proc_lock, flags);
>  	adapter->main_locked = false;
>  	spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
> @@ -2999,8 +2997,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
>  			return ERR_PTR(-EINVAL);
>  		}
>  
> -		priv = mwifiex_get_unused_priv_by_bss_type(
> -						adapter, MWIFIEX_BSS_TYPE_STA);
> +		priv = mwifiex_get_unused_priv(adapter);
>  		if (!priv) {
>  			mwifiex_dbg(adapter, ERROR,
>  				    "could not get free private struct\n");
> @@ -3029,8 +3026,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
>  			return ERR_PTR(-EINVAL);
>  		}
>  
> -		priv = mwifiex_get_unused_priv_by_bss_type(
> -						adapter, MWIFIEX_BSS_TYPE_UAP);
> +		priv = mwifiex_get_unused_priv(adapter);
>  		if (!priv) {
>  			mwifiex_dbg(adapter, ERROR,
>  				    "could not get free private struct\n");
> @@ -3056,8 +3052,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
>  			return ERR_PTR(-EINVAL);
>  		}
>  
> -		priv = mwifiex_get_unused_priv_by_bss_type(
> -						adapter, MWIFIEX_BSS_TYPE_P2P);
> +		priv = mwifiex_get_unused_priv(adapter);
>  		if (!priv) {
>  			mwifiex_dbg(adapter, ERROR,
>  				    "could not get free private struct\n");
> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 4f814110f750e..d91351384c6bb 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -496,8 +496,7 @@ int mwifiex_process_event(struct mwifiex_adapter *adapter)
>  							(u16) eventcause;
>  
>  	/* Get BSS number and corresponding priv */
> -	priv = mwifiex_get_priv_by_id(adapter, EVENT_GET_BSS_NUM(eventcause),
> -				      EVENT_GET_BSS_TYPE(eventcause));
> +	priv = mwifiex_get_priv_by_id(adapter, EVENT_GET_BSS_NUM(eventcause));
>  	if (!priv)
>  		priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>  
> @@ -847,8 +846,7 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
>  
>  	/* Get BSS number and corresponding priv */
>  	priv = mwifiex_get_priv_by_id(adapter,
> -			     HostCmd_GET_BSS_NO(le16_to_cpu(resp->seq_num)),
> -			     HostCmd_GET_BSS_TYPE(le16_to_cpu(resp->seq_num)));
> +			     HostCmd_GET_BSS_NO(le16_to_cpu(resp->seq_num)));
>  	if (!priv)
>  		priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>  	/* Clear RET_BIT from HostCmd */
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 7cb90a6a8ccab..888f2801d6f2a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -85,6 +85,7 @@ static int mwifiex_register(void *card, struct device *dev,
>  		if (!adapter->priv[i])
>  			goto error;
>  
> +		adapter->priv[i]->bss_num = i;
>  		adapter->priv[i]->adapter = adapter;
>  		adapter->priv_num++;
>  	}
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 541bc50a9561c..2938e55a38d79 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1297,20 +1297,12 @@ mwifiex_copy_rates(u8 *dest, u32 pos, u8 *src, int len)
>   * upon the BSS type and BSS number.
>   */
>  static inline struct mwifiex_private *
> -mwifiex_get_priv_by_id(struct mwifiex_adapter *adapter,
> -		       u8 bss_num, u8 bss_type)
> +mwifiex_get_priv_by_id(struct mwifiex_adapter *adapter, u8 bss_num)
>  {
> -	int i;
> -
> -	for (i = 0; i < adapter->priv_num; i++) {
> -		if (adapter->priv[i]->bss_mode == NL80211_IFTYPE_UNSPECIFIED)
> -			continue;
> +	if (bss_num >= MWIFIEX_MAX_BSS_NUM)
> +		return NULL;
>  
> -		if ((adapter->priv[i]->bss_num == bss_num) &&
> -		    (adapter->priv[i]->bss_type == bss_type))
> -			break;
> -	}
> -	return ((i < adapter->priv_num) ? adapter->priv[i] : NULL);
> +	return adapter->priv[bss_num];
>  }
>  
>  /*
> @@ -1332,47 +1324,19 @@ mwifiex_get_priv(struct mwifiex_adapter *adapter,
>  	return ((i < adapter->priv_num) ? adapter->priv[i] : NULL);
>  }
>  
> -/*
> - * This function checks available bss_num when adding new interface or
> - * changing interface type.
> - */
> -static inline u8
> -mwifiex_get_unused_bss_num(struct mwifiex_adapter *adapter, u8 bss_type)
> -{
> -	u8 i, j;
> -	int index[MWIFIEX_MAX_BSS_NUM];
> -
> -	memset(index, 0, sizeof(index));
> -	for (i = 0; i < adapter->priv_num; i++)
> -		if (adapter->priv[i]->bss_type == bss_type &&
> -		    !(adapter->priv[i]->bss_mode ==
> -		      NL80211_IFTYPE_UNSPECIFIED)) {
> -			index[adapter->priv[i]->bss_num] = 1;
> -		}
> -	for (j = 0; j < MWIFIEX_MAX_BSS_NUM; j++)
> -		if (!index[j])
> -			return j;
> -	return -1;
> -}
> -
>  /*
>   * This function returns the first available unused private structure pointer.
>   */
>  static inline struct mwifiex_private *
> -mwifiex_get_unused_priv_by_bss_type(struct mwifiex_adapter *adapter,
> -				    u8 bss_type)
> +mwifiex_get_unused_priv(struct mwifiex_adapter *adapter)
>  {
> -	u8 i;
> +	int i;
>  
>  	for (i = 0; i < adapter->priv_num; i++)
> -		if (adapter->priv[i]->bss_mode ==
> -		   NL80211_IFTYPE_UNSPECIFIED) {
> -			adapter->priv[i]->bss_num =
> -			mwifiex_get_unused_bss_num(adapter, bss_type);
> -			break;
> -		}
> +		if (adapter->priv[i]->bss_mode == NL80211_IFTYPE_UNSPECIFIED)
> +			return adapter->priv[i];
>  
> -	return ((i < adapter->priv_num) ? adapter->priv[i] : NULL);
> +	return NULL;
>  }
>  
>  /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> index b5f3821a6a8f2..15f057d010a3d 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> @@ -456,8 +456,7 @@ void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
>  		for (i = 0; i < intf_num; i++) {
>  			bss_type = grp_info->bss_type_numlist[i] >> 4;
>  			bss_num = grp_info->bss_type_numlist[i] & BSS_NUM_MASK;
> -			intf_priv = mwifiex_get_priv_by_id(adapter, bss_num,
> -							   bss_type);
> +			intf_priv = mwifiex_get_priv_by_id(adapter, bss_num);
>  			if (!intf_priv) {
>  				mwifiex_dbg(adapter, ERROR,
>  					    "Invalid bss_type bss_num\t"
> diff --git a/drivers/net/wireless/marvell/mwifiex/txrx.c b/drivers/net/wireless/marvell/mwifiex/txrx.c
> index f44e22f245110..21cfee3290377 100644
> --- a/drivers/net/wireless/marvell/mwifiex/txrx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/txrx.c
> @@ -31,8 +31,7 @@ int mwifiex_handle_rx_packet(struct mwifiex_adapter *adapter,
>  
>  	local_rx_pd = (struct rxpd *) (skb->data);
>  	/* Get the BSS number from rxpd, get corresponding priv */
> -	priv = mwifiex_get_priv_by_id(adapter, local_rx_pd->bss_num &
> -				      BSS_NUM_MASK, local_rx_pd->bss_type);
> +	priv = mwifiex_get_priv_by_id(adapter, local_rx_pd->bss_num & BSS_NUM_MASK);
>  	if (!priv)
>  		priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
>  
> @@ -165,8 +164,7 @@ static int mwifiex_host_to_card(struct mwifiex_adapter *adapter,
>  	struct mwifiex_txinfo *tx_info;
>  
>  	tx_info = MWIFIEX_SKB_TXCB(skb);
> -	priv = mwifiex_get_priv_by_id(adapter, tx_info->bss_num,
> -				      tx_info->bss_type);
> +	priv = mwifiex_get_priv_by_id(adapter, tx_info->bss_num);
>  	if (!priv) {
>  		mwifiex_dbg(adapter, ERROR,
>  			    "data: priv not found. Drop TX packet\n");
> @@ -281,8 +279,7 @@ int mwifiex_write_data_complete(struct mwifiex_adapter *adapter,
>  		return 0;
>  
>  	tx_info = MWIFIEX_SKB_TXCB(skb);
> -	priv = mwifiex_get_priv_by_id(adapter, tx_info->bss_num,
> -				      tx_info->bss_type);
> +	priv = mwifiex_get_priv_by_id(adapter, tx_info->bss_num);
>  	if (!priv)
>  		goto done;
>  
> 
> -- 
> 2.39.2
> 
> 

-- 
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 |