[PATCH] ath10k: improve vdev map handling.

[PATCH] ath10k: improve vdev map handling.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Check vdev map has space before calling ffs,
fix invalid cleanup in failure to create vdev
case.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This is compile-tested only.

 drivers/net/wireless/ath/ath10k/mac.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e2c01dc..d461f05 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -594,14 +594,14 @@ static int ath10k_monitor_vdev_create(struct ath10k *ar)
 
 	lockdep_assert_held(&ar->conf_mutex);
 
-	bit = ffs(ar->free_vdev_map);
-	if (bit == 0) {
+	if (!ar->free_vdev_map) {
 		ath10k_warn("failed to find free vdev id for monitor vdev\n");
 		return -ENOMEM;
 	}
 
+	bit = ffs(ar->free_vdev_map);
+
 	ar->monitor_vdev_id = bit - 1;
-	ar->free_vdev_map &= ~(1 << ar->monitor_vdev_id);
 
 	ret = ath10k_wmi_vdev_create(ar, ar->monitor_vdev_id,
 				     WMI_VDEV_TYPE_MONITOR,
@@ -609,20 +609,14 @@ static int ath10k_monitor_vdev_create(struct ath10k *ar)
 	if (ret) {
 		ath10k_warn("failed to request monitor vdev %i creation: %d\n",
 			    ar->monitor_vdev_id, ret);
-		goto vdev_fail;
+		return ret;
 	}
 
+	ar->free_vdev_map &= ~(1 << ar->monitor_vdev_id);
 	ath10k_dbg(ATH10K_DBG_MAC, "mac monitor vdev %d created\n",
 		   ar->monitor_vdev_id);
 
 	return 0;
-
-vdev_fail:
-	/*
-	 * Restore the ID to the global map.
-	 */
-	ar->free_vdev_map |= 1 << (ar->monitor_vdev_id);
-	return ret;
 }
 
 static int ath10k_monitor_vdev_delete(struct ath10k *ar)
@@ -638,7 +632,7 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar)
 		return ret;
 	}
 
-	ar->free_vdev_map |= 1 << (ar->monitor_vdev_id);
+	ar->free_vdev_map |= (1 << ar->monitor_vdev_id);
 
 	ath10k_dbg(ATH10K_DBG_MAC, "mac monitor vdev %d deleted\n",
 		   ar->monitor_vdev_id);
@@ -2622,11 +2616,12 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 	INIT_WORK(&arvif->wep_key_work, ath10k_tx_wep_key_work);
 	INIT_LIST_HEAD(&arvif->list);
 
-	bit = ffs(ar->free_vdev_map);
-	if (bit == 0) {
+	if (!ar->free_vdev_map) {
+		ath10k_warn("Free vdev map is empty, no more interfaces allowed.\n");
 		ret = -EBUSY;
 		goto err;
 	}
+	bit = ffs(ar->free_vdev_map);
 
 	arvif->vdev_id = bit - 1;
 	arvif->vdev_subtype = WMI_VDEV_SUBTYPE_NONE;
@@ -2669,7 +2664,7 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 		goto err;
 	}
 
-	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
+	ar->free_vdev_map &= ~(1 << arvif->vdev_id);
 	list_add(&arvif->list, &ar->arvifs);
 
 	vdev_param = ar->wmi.vdev_param->def_keyid;
@@ -2762,7 +2757,7 @@ err_peer_delete:
 
 err_vdev_delete:
 	ath10k_wmi_vdev_delete(ar, arvif->vdev_id);
-	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
+	ar->free_vdev_map |= (1 << arvif->vdev_id);
 	list_del(&arvif->list);
 
 err:
@@ -2792,7 +2787,7 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
 	}
 	spin_unlock_bh(&ar->data_lock);
 
-	ar->free_vdev_map |= 1 << (arvif->vdev_id);
+	ar->free_vdev_map |= (1 << arvif->vdev_id);
 	list_del(&arvif->list);
 
 	if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
-- 
1.7.11.7

Re: [PATCH] ath10k: improve vdev map handling.

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Check vdev map has space before calling ffs,
> fix invalid cleanup in failure to create vdev
> case.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

Why? What motivated you to write this? If you saw a bug, it would be
good to document the bug in the commit log.

> This is compile-tested only.

It's two weeks since you posted this, sorry for taking so long. But have
you managed to test this by now?

-- 
Kalle Valo

Re: [PATCH] ath10k: improve vdev map handling.

[Ben Greear]

On 05/16/2014 06:18 AM, Kalle Valo wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Check vdev map has space before calling ffs,
>> fix invalid cleanup in failure to create vdev
>> case.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>
> Why? What motivated you to write this? If you saw a bug, it would be
> good to document the bug in the commit log.
>
>> This is compile-tested only.
>
> It's two weeks since you posted this, sorry for taking so long. But have
> you managed to test this by now?

The code is cleaner with my patch, and it makes it easier to
use the 64-bit version of ffs.

I have tested this extensively in my tree with the 64-bit version
of ffs so that I can have more than 32 vdevs.

It does fix a problem I found through code inspection as well,
when it did not properly release a slot in a failure path.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath10k: improve vdev map handling.

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Check vdev map has space before calling ffs,
> fix invalid cleanup in failure to create vdev
> case.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

[...]

> @@ -594,14 +594,14 @@ static int ath10k_monitor_vdev_create(struct ath10k *ar)
>  
>  	lockdep_assert_held(&ar->conf_mutex);
>  
> -	bit = ffs(ar->free_vdev_map);
> -	if (bit == 0) {
> +	if (!ar->free_vdev_map) {

As we are using ar->free_vdev_map as a bitmap, I think !foo is just
confusing. Wouldn't '== 0' make more sense here?

> @@ -638,7 +632,7 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar)
>  		return ret;
>  	}
>  
> -	ar->free_vdev_map |= 1 << (ar->monitor_vdev_id);
> +	ar->free_vdev_map |= (1 << ar->monitor_vdev_id);

Aren't the parentheses useless here?

> @@ -2622,11 +2616,12 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
>  	INIT_WORK(&arvif->wep_key_work, ath10k_tx_wep_key_work);
>  	INIT_LIST_HEAD(&arvif->list);
>  
> -	bit = ffs(ar->free_vdev_map);
> -	if (bit == 0) {
> +	if (!ar->free_vdev_map) {

Ditto about '==0'.

> +		ath10k_warn("Free vdev map is empty, no more interfaces allowed.\n");
>  		ret = -EBUSY;
>  		goto err;
>  	}
> +	bit = ffs(ar->free_vdev_map);

Empty line after '}', please.

> @@ -2669,7 +2664,7 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
>  		goto err;
>  	}
>  
> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
> +	ar->free_vdev_map &= ~(1 << arvif->vdev_id);

Why remove the BIT()? Not that it matters much, I just think it's easier
to read when BIT() macro is used. Would be good to convert all cases to
use BIT anyway, but that's for a separate patch.

>  err_vdev_delete:
>  	ath10k_wmi_vdev_delete(ar, arvif->vdev_id);
> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
> +	ar->free_vdev_map |= (1 << arvif->vdev_id);

Again why remove BIT()?

> @@ -2792,7 +2787,7 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
>  	}
>  	spin_unlock_bh(&ar->data_lock);
>  
> -	ar->free_vdev_map |= 1 << (arvif->vdev_id);
> +	ar->free_vdev_map |= (1 << arvif->vdev_id);

Do we need the parenthesis?

-- 
Kalle Valo

Re: [PATCH] ath10k: improve vdev map handling.

[Ben Greear]

On 05/16/2014 06:37 AM, Kalle Valo wrote:

>> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
>> +	ar->free_vdev_map &= ~(1 << arvif->vdev_id);
> 
> Why remove the BIT()? Not that it matters much, I just think it's easier
> to read when BIT() macro is used. Would be good to convert all cases to
> use BIT anyway, but that's for a separate patch.

BIT doesn't work on 64-bit numbers (ie, if vdev_id > 31), and it takes a long time to
figure out exactly what it does (try grepping for BIT).  Open-coding means much
easier to fully understand the code.


>>  err_vdev_delete:
>>  	ath10k_wmi_vdev_delete(ar, arvif->vdev_id);
>> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
>> +	ar->free_vdev_map |= (1 << arvif->vdev_id);
> 
> Again why remove BIT()?
> 
>> @@ -2792,7 +2787,7 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
>>  	}
>>  	spin_unlock_bh(&ar->data_lock);
>>  
>> -	ar->free_vdev_map |= 1 << (arvif->vdev_id);
>> +	ar->free_vdev_map |= (1 << arvif->vdev_id);
> 
> Do we need the parenthesis?

No, though I like them visually.  It's at least more useful than
the previous placement.

I can respin the patch w/out them and with the == 0 and such.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath10k: improve vdev map handling.

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

> On 05/16/2014 06:37 AM, Kalle Valo wrote:
>
>>> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
>>> +	ar->free_vdev_map &= ~(1 << arvif->vdev_id);
>> 
>> Why remove the BIT()? Not that it matters much, I just think it's easier
>> to read when BIT() macro is used. Would be good to convert all cases to
>> use BIT anyway, but that's for a separate patch.
>
> BIT doesn't work on 64-bit numbers (ie, if vdev_id > 31)

Oh, I didn't know that. Too bad, but then removing it makes sense.

> and it takes a long time to figure out exactly what it does (try
> grepping for BIT). Open-coding means much easier to fully understand
> the code.

All Linux engineers should know what BIT() does. If not, they should
learn that ;)

>>> -	ar->free_vdev_map |= 1 << (arvif->vdev_id);
>>> +	ar->free_vdev_map |= (1 << arvif->vdev_id);
>> 
>> Do we need the parenthesis?
>
> No, though I like them visually.  It's at least more useful than
> the previous placement.
>
> I can respin the patch w/out them and with the == 0 and such.

Thanks.

-- 
Kalle Valo

Re: [PATCH] ath10k: improve vdev map handling.

[Ben Greear]

On 05/16/2014 07:06 AM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
> 
>> On 05/16/2014 06:37 AM, Kalle Valo wrote:
>>
>>>> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
>>>> +	ar->free_vdev_map &= ~(1 << arvif->vdev_id);
>>>
>>> Why remove the BIT()? Not that it matters much, I just think it's easier
>>> to read when BIT() macro is used. Would be good to convert all cases to
>>> use BIT anyway, but that's for a separate patch.
>>
>> BIT doesn't work on 64-bit numbers (ie, if vdev_id > 31)
> 
> Oh, I didn't know that. Too bad, but then removing it makes sense.
> 
>> and it takes a long time to figure out exactly what it does (try
>> grepping for BIT). Open-coding means much easier to fully understand
>> the code.
> 
> All Linux engineers should know what BIT() does. If not, they should
> learn that ;)

Yeah, but see your comment above :P

Pain in the ass to track down the difference between (1 << x) and (1LLU << x),
and even worse when it's hidden behind a macro.

Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com