diff mbox series

[1/5] wifi: ath11k: refactor transmitted arvif retrieval

Message ID 20250114223612.2979310-2-quic_alokad@quicinc.com
State New
Headers show
Series wifi:ath11k/ath12k: refactor tx_arvif retrieval | expand

Commit Message

Aloka Dixit Jan. 14, 2025, 10:36 p.m. UTC 
Create a new function ath11k_mac_get_tx_arvif() to retrieve 'arvif'
for the transmitted interface of the MBSSID set. This will help
modifying the same code path to reflect mac80211 data structure
changes to support MLO. This also fixes an issue in
ath11k_mac_update_vif_chan() where tx_arvif is not reset to NULL
inside for loop during each iteration.

Signed-off-by: Aloka Dixit <quic_alokad@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/mac.c | 34 +++++++++++++--------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Sidhanta Sahu Jan. 15, 2025, 7:36 p.m. UTC | #1 
>   
> +static struct ath11k_vif *ath11k_mac_get_tx_arvif(struct ath11k_vif *arvif)
> +{
> +	if (arvif->vif->mbssid_tx_vif)
> +		return ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
> +
> +	return NULL;
> +}
> +
>   static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
>   {
>   	struct ath11k_vif *tx_arvif;
> @@ -1538,7 +1546,7 @@ static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
>   	u32 params = 0;
>   	u8 i = 0;
>   
> -	tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
> +	tx_arvif = ath11k_mac_get_tx_arvif(arvif);

ath11k_mac_get_tx_arvif can return NULL, below, we are accessing 
tx_arvif without a NULL check. Shouldn't we add a null check wherever 
applicable to prevent potential issues?

>   
>   	beacons = ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
>   							 tx_arvif->vif, 0);
> @@ -1597,7 +1605,7 @@ static int ath11k_mac_setup_bcn_tmpl_mbssid(struct ath11k_vif *arvif)
>   	int ret;
>   
>   	if (vif->mbssid_tx_vif) {
> -		tx_arvif = ath11k_vif_to_arvif(vif->mbssid_tx_vif);
> +		tx_arvif = ath11k_mac_get_tx_arvif(arvif);
>   		if (tx_arvif != arvif) {
>   			ar = tx_arvif->ar;
>   			ab = ar->ab;
> @@ -1674,7 +1682,7 @@ static void ath11k_control_beaconing(struct ath11k_vif *arvif,
>   				     struct ieee80211_bss_conf *info)
>   {
>   	struct ath11k *ar = arvif->ar;
> -	struct ath11k_vif *tx_arvif = NULL;
> +	struct ath11k_vif *tx_arvif;
>   	int ret = 0;
>   
>   	lockdep_assert_held(&arvif->ar->conf_mutex);
> @@ -1701,9 +1709,7 @@ static void ath11k_control_beaconing(struct ath11k_vif *arvif,
>   
>   	ether_addr_copy(arvif->bssid, info->bssid);
>   
> -	if (arvif->vif->mbssid_tx_vif)
> -		tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
> -
> +	tx_arvif = ath11k_mac_get_tx_arvif(arvif);
>   	ret = ath11k_wmi_vdev_up(arvif->ar, arvif->vdev_id, arvif->aid,
>   				 arvif->bssid,
>   				 tx_arvif ? tx_arvif->bssid : NULL,
> @@ -6333,14 +6339,12 @@ static int ath11k_mac_setup_vdev_params_mbssid(struct ath11k_vif *arvif,
>   	struct ieee80211_vif *tx_vif;
>   
>   	*tx_vdev_id = 0;
> -	tx_vif = arvif->vif->mbssid_tx_vif;
> -	if (!tx_vif) {
> +	tx_arvif = ath11k_mac_get_tx_arvif(arvif);
> +	if (!tx_arvif) {
>   		*flags = WMI_HOST_VDEV_FLAGS_NON_MBSSID_AP;
>   		return 0;
>   	}
>   
> -	tx_arvif = ath11k_vif_to_arvif(tx_vif);
> -
>   	if (arvif->vif->bss_conf.nontransmitted) {
>   		if (ar->hw->wiphy != ieee80211_vif_to_wdev(tx_vif)->wiphy)
>   			return -EINVAL;
> @@ -7306,8 +7310,7 @@ ath11k_mac_update_vif_chan(struct ath11k *ar,
>   			   int n_vifs)
>   {
>   	struct ath11k_base *ab = ar->ab;
> -	struct ath11k_vif *arvif, *tx_arvif = NULL;
> -	struct ieee80211_vif *mbssid_tx_vif;
> +	struct ath11k_vif *arvif, *tx_arvif;
>   	int ret;
>   	int i;
>   	bool monitor_vif = false;
> @@ -7361,10 +7364,7 @@ ath11k_mac_update_vif_chan(struct ath11k *ar,
>   			ath11k_warn(ab, "failed to update bcn tmpl during csa: %d\n",
>   				    ret);
>   
> -		mbssid_tx_vif = arvif->vif->mbssid_tx_vif;
> -		if (mbssid_tx_vif)
> -			tx_arvif = ath11k_vif_to_arvif(mbssid_tx_vif);
> -
> +		tx_arvif = ath11k_mac_get_tx_arvif(arvif);
>   		ret = ath11k_wmi_vdev_up(arvif->ar, arvif->vdev_id, arvif->aid,
>   					 arvif->bssid,
>   					 tx_arvif ? tx_arvif->bssid : NULL,
kernel test robot Jan. 15, 2025, 11:16 p.m. UTC | #2 
Hi Aloka,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0c5fcd9069dd5f984e39820629acbfbe0f1b4256]

url:    https://github.com/intel-lab-lkp/linux/commits/Aloka-Dixit/wifi-ath11k-refactor-transmitted-arvif-retrieval/20250115-063922
base:   0c5fcd9069dd5f984e39820629acbfbe0f1b4256
patch link:    https://lore.kernel.org/r/20250114223612.2979310-2-quic_alokad%40quicinc.com
patch subject: [PATCH 1/5] wifi: ath11k: refactor transmitted arvif retrieval
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20250116/202501160626.Jbb3GHnk-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250116/202501160626.Jbb3GHnk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501160626.Jbb3GHnk-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/wireless/ath/ath11k/mac.c:6349:46: warning: variable 'tx_vif' is uninitialized when used here [-Wuninitialized]
    6349 |                 if (ar->hw->wiphy != ieee80211_vif_to_wdev(tx_vif)->wiphy)
         |                                                            ^~~~~~
   drivers/net/wireless/ath/ath11k/mac.c:6339:30: note: initialize the variable 'tx_vif' to silence this warning
    6339 |         struct ieee80211_vif *tx_vif;
         |                                     ^
         |                                      = NULL
   1 warning generated.


vim +/tx_vif +6349 drivers/net/wireless/ath/ath11k/mac.c

d5c65159f28953 Kalle Valo  2019-11-23  6333  
5a81610acf66c4 Aloka Dixit 2023-05-05  6334  static int ath11k_mac_setup_vdev_params_mbssid(struct ath11k_vif *arvif,
5a81610acf66c4 Aloka Dixit 2023-05-05  6335  					       u32 *flags, u32 *tx_vdev_id)
5a81610acf66c4 Aloka Dixit 2023-05-05  6336  {
5a81610acf66c4 Aloka Dixit 2023-05-05  6337  	struct ath11k *ar = arvif->ar;
5a81610acf66c4 Aloka Dixit 2023-05-05  6338  	struct ath11k_vif *tx_arvif;
5a81610acf66c4 Aloka Dixit 2023-05-05  6339  	struct ieee80211_vif *tx_vif;
5a81610acf66c4 Aloka Dixit 2023-05-05  6340  
5a81610acf66c4 Aloka Dixit 2023-05-05  6341  	*tx_vdev_id = 0;
72f88bc503a76c Aloka Dixit 2025-01-14  6342  	tx_arvif = ath11k_mac_get_tx_arvif(arvif);
72f88bc503a76c Aloka Dixit 2025-01-14  6343  	if (!tx_arvif) {
5a81610acf66c4 Aloka Dixit 2023-05-05  6344  		*flags = WMI_HOST_VDEV_FLAGS_NON_MBSSID_AP;
5a81610acf66c4 Aloka Dixit 2023-05-05  6345  		return 0;
5a81610acf66c4 Aloka Dixit 2023-05-05  6346  	}
5a81610acf66c4 Aloka Dixit 2023-05-05  6347  
5a81610acf66c4 Aloka Dixit 2023-05-05  6348  	if (arvif->vif->bss_conf.nontransmitted) {
5a81610acf66c4 Aloka Dixit 2023-05-05 @6349  		if (ar->hw->wiphy != ieee80211_vif_to_wdev(tx_vif)->wiphy)
5a81610acf66c4 Aloka Dixit 2023-05-05  6350  			return -EINVAL;
5a81610acf66c4 Aloka Dixit 2023-05-05  6351  
5a81610acf66c4 Aloka Dixit 2023-05-05  6352  		*flags = WMI_HOST_VDEV_FLAGS_NON_TRANSMIT_AP;
5a81610acf66c4 Aloka Dixit 2023-05-05  6353  		*tx_vdev_id = ath11k_vif_to_arvif(tx_vif)->vdev_id;
5a81610acf66c4 Aloka Dixit 2023-05-05  6354  	} else if (tx_arvif == arvif) {
5a81610acf66c4 Aloka Dixit 2023-05-05  6355  		*flags = WMI_HOST_VDEV_FLAGS_TRANSMIT_AP;
5a81610acf66c4 Aloka Dixit 2023-05-05  6356  	} else {
5a81610acf66c4 Aloka Dixit 2023-05-05  6357  		return -EINVAL;
5a81610acf66c4 Aloka Dixit 2023-05-05  6358  	}
5a81610acf66c4 Aloka Dixit 2023-05-05  6359  
5a81610acf66c4 Aloka Dixit 2023-05-05  6360  	if (arvif->vif->bss_conf.ema_ap)
5a81610acf66c4 Aloka Dixit 2023-05-05  6361  		*flags |= WMI_HOST_VDEV_FLAGS_EMA_MODE;
5a81610acf66c4 Aloka Dixit 2023-05-05  6362  
5a81610acf66c4 Aloka Dixit 2023-05-05  6363  	return 0;
5a81610acf66c4 Aloka Dixit 2023-05-05  6364  }
5a81610acf66c4 Aloka Dixit 2023-05-05  6365
Aloka Dixit Jan. 18, 2025, 12:36 a.m. UTC | #3 
On 1/15/2025 11:36 AM, Sidhanta Sahu wrote:
>
>>   +static struct ath11k_vif *ath11k_mac_get_tx_arvif(struct 
>> ath11k_vif *arvif)
>> +{
>> +    if (arvif->vif->mbssid_tx_vif)
>> +        return ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
>> +
>> +    return NULL;
>> +}
>> +
>>   static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
>>   {
>>       struct ath11k_vif *tx_arvif;
>> @@ -1538,7 +1546,7 @@ static int ath11k_mac_setup_bcn_tmpl_ema(struct 
>> ath11k_vif *arvif)
>>       u32 params = 0;
>>       u8 i = 0;
>>   -    tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
>> +    tx_arvif = ath11k_mac_get_tx_arvif(arvif);
>
> ath11k_mac_get_tx_arvif can return NULL, below, we are accessing 
> tx_arvif without a NULL check. Shouldn't we add a null check wherever 
> applicable to prevent potential issues?
>
>>         beacons = 
>> ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
>>                                tx_arvif->vif, 0);

ath11k_mac_setup_bcn_tmpl_ema() gets called only when tx_arvif is non-NULL.
Sidhanta Sahu Jan. 18, 2025, 12:44 a.m. UTC | #4 
On 1/17/2025 4:36 PM, Aloka Dixit wrote:
> 
> On 1/15/2025 11:36 AM, Sidhanta Sahu wrote:
>>
>>>   +static struct ath11k_vif *ath11k_mac_get_tx_arvif(struct 
>>> ath11k_vif *arvif)
>>> +{
>>> +    if (arvif->vif->mbssid_tx_vif)
>>> +        return ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
>>> +
>>> +    return NULL;
>>> +}
>>> +
>>>   static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
>>>   {
>>>       struct ath11k_vif *tx_arvif;
>>> @@ -1538,7 +1546,7 @@ static int ath11k_mac_setup_bcn_tmpl_ema(struct 
>>> ath11k_vif *arvif)
>>>       u32 params = 0;
>>>       u8 i = 0;
>>>   -    tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
>>> +    tx_arvif = ath11k_mac_get_tx_arvif(arvif);
>>
>> ath11k_mac_get_tx_arvif can return NULL, below, we are accessing 
>> tx_arvif without a NULL check. Shouldn't we add a null check wherever 
>> applicable to prevent potential issues?
>>
>>>         beacons = 
>>> ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
>>>                                tx_arvif->vif, 0);
> 
> ath11k_mac_setup_bcn_tmpl_ema() gets called only when tx_arvif is non-NULL.

If tx_arvif is not NULL (and known already), Is it still required to 
call `ath11k_mac_get_tx_arvif` under `ath11k_mac_setup_bcn_tmpl_ema`?

> 
>
Aloka Dixit Jan. 19, 2025, 4:57 a.m. UTC | #5 
On 1/17/2025 4:44 PM, Sidhanta Sahu wrote:
>
>
> On 1/17/2025 4:36 PM, Aloka Dixit wrote:
>>
>> On 1/15/2025 11:36 AM, Sidhanta Sahu wrote:
>>>
>>>>   +static struct ath11k_vif *ath11k_mac_get_tx_arvif(struct 
>>>> ath11k_vif *arvif)
>>>> +{
>>>> +    if (arvif->vif->mbssid_tx_vif)
>>>> +        return ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>>   static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
>>>>   {
>>>>       struct ath11k_vif *tx_arvif;
>>>> @@ -1538,7 +1546,7 @@ static int 
>>>> ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
>>>>       u32 params = 0;
>>>>       u8 i = 0;
>>>>   -    tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
>>>> +    tx_arvif = ath11k_mac_get_tx_arvif(arvif);
>>>
>>> ath11k_mac_get_tx_arvif can return NULL, below, we are accessing 
>>> tx_arvif without a NULL check. Shouldn't we add a null check 
>>> wherever applicable to prevent potential issues?
>>>
>>>>         beacons = 
>>>> ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
>>>>                                tx_arvif->vif, 0);
>>
>> ath11k_mac_setup_bcn_tmpl_ema() gets called only when tx_arvif is 
>> non-NULL.
>
> If tx_arvif is not NULL (and known already), Is it still required to 
> call `ath11k_mac_get_tx_arvif` under `ath11k_mac_setup_bcn_tmpl_ema`?


Yes, as per the current code it still needs to retrieve tx_arvif to be used.

This patch 1/5 only creates a new function to refactor existing repeated 
code.

The next patch,

https://patchwork.kernel.org/project/linux-wireless/patch/20250114223612.2979310-3-quic_alokad@quicinc.com/,

adds few more changes on the same code.
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 1556392f7ad4..ebd5a9a7b6c3 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -1,7 +1,7 @@ 
 // SPDX-License-Identifier: BSD-3-Clause-Clear
 /*
  * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <net/mac80211.h>
@@ -1529,6 +1529,14 @@  static int ath11k_mac_set_vif_params(struct ath11k_vif *arvif,
 	return ret;
 }
 
+static struct ath11k_vif *ath11k_mac_get_tx_arvif(struct ath11k_vif *arvif)
+{
+	if (arvif->vif->mbssid_tx_vif)
+		return ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
+
+	return NULL;
+}
+
 static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
 {
 	struct ath11k_vif *tx_arvif;
@@ -1538,7 +1546,7 @@  static int ath11k_mac_setup_bcn_tmpl_ema(struct ath11k_vif *arvif)
 	u32 params = 0;
 	u8 i = 0;
 
-	tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
+	tx_arvif = ath11k_mac_get_tx_arvif(arvif);
 
 	beacons = ieee80211_beacon_get_template_ema_list(tx_arvif->ar->hw,
 							 tx_arvif->vif, 0);
@@ -1597,7 +1605,7 @@  static int ath11k_mac_setup_bcn_tmpl_mbssid(struct ath11k_vif *arvif)
 	int ret;
 
 	if (vif->mbssid_tx_vif) {
-		tx_arvif = ath11k_vif_to_arvif(vif->mbssid_tx_vif);
+		tx_arvif = ath11k_mac_get_tx_arvif(arvif);
 		if (tx_arvif != arvif) {
 			ar = tx_arvif->ar;
 			ab = ar->ab;
@@ -1674,7 +1682,7 @@  static void ath11k_control_beaconing(struct ath11k_vif *arvif,
 				     struct ieee80211_bss_conf *info)
 {
 	struct ath11k *ar = arvif->ar;
-	struct ath11k_vif *tx_arvif = NULL;
+	struct ath11k_vif *tx_arvif;
 	int ret = 0;
 
 	lockdep_assert_held(&arvif->ar->conf_mutex);
@@ -1701,9 +1709,7 @@  static void ath11k_control_beaconing(struct ath11k_vif *arvif,
 
 	ether_addr_copy(arvif->bssid, info->bssid);
 
-	if (arvif->vif->mbssid_tx_vif)
-		tx_arvif = ath11k_vif_to_arvif(arvif->vif->mbssid_tx_vif);
-
+	tx_arvif = ath11k_mac_get_tx_arvif(arvif);
 	ret = ath11k_wmi_vdev_up(arvif->ar, arvif->vdev_id, arvif->aid,
 				 arvif->bssid,
 				 tx_arvif ? tx_arvif->bssid : NULL,
@@ -6333,14 +6339,12 @@  static int ath11k_mac_setup_vdev_params_mbssid(struct ath11k_vif *arvif,
 	struct ieee80211_vif *tx_vif;
 
 	*tx_vdev_id = 0;
-	tx_vif = arvif->vif->mbssid_tx_vif;
-	if (!tx_vif) {
+	tx_arvif = ath11k_mac_get_tx_arvif(arvif);
+	if (!tx_arvif) {
 		*flags = WMI_HOST_VDEV_FLAGS_NON_MBSSID_AP;
 		return 0;
 	}
 
-	tx_arvif = ath11k_vif_to_arvif(tx_vif);
-
 	if (arvif->vif->bss_conf.nontransmitted) {
 		if (ar->hw->wiphy != ieee80211_vif_to_wdev(tx_vif)->wiphy)
 			return -EINVAL;
@@ -7306,8 +7310,7 @@  ath11k_mac_update_vif_chan(struct ath11k *ar,
 			   int n_vifs)
 {
 	struct ath11k_base *ab = ar->ab;
-	struct ath11k_vif *arvif, *tx_arvif = NULL;
-	struct ieee80211_vif *mbssid_tx_vif;
+	struct ath11k_vif *arvif, *tx_arvif;
 	int ret;
 	int i;
 	bool monitor_vif = false;
@@ -7361,10 +7364,7 @@  ath11k_mac_update_vif_chan(struct ath11k *ar,
 			ath11k_warn(ab, "failed to update bcn tmpl during csa: %d\n",
 				    ret);
 
-		mbssid_tx_vif = arvif->vif->mbssid_tx_vif;
-		if (mbssid_tx_vif)
-			tx_arvif = ath11k_vif_to_arvif(mbssid_tx_vif);
-
+		tx_arvif = ath11k_mac_get_tx_arvif(arvif);
 		ret = ath11k_wmi_vdev_up(arvif->ar, arvif->vdev_id, arvif->aid,
 					 arvif->bssid,
 					 tx_arvif ? tx_arvif->bssid : NULL,