* [PATCH] ath10k: improve vdev map handling.
@ 2014-04-30 18:22 greearb
2014-05-16 13:18 ` Kalle Valo
2014-05-16 13:37 ` Kalle Valo
0 siblings, 2 replies; 7+ messages in thread
From: greearb @ 2014-04-30 18:22 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Ben Greear
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ath10k: improve vdev map handling.
2014-04-30 18:22 [PATCH] ath10k: improve vdev map handling greearb
@ 2014-05-16 13:18 ` Kalle Valo
2014-05-16 13:26 ` Ben Greear
2014-05-16 13:37 ` Kalle Valo
1 sibling, 1 reply; 7+ messages in thread
From: Kalle Valo @ 2014-05-16 13:18 UTC (permalink / raw)
To: greearb; +Cc: ath10k, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ath10k: improve vdev map handling.
2014-05-16 13:18 ` Kalle Valo
@ 2014-05-16 13:26 ` Ben Greear
0 siblings, 0 replies; 7+ messages in thread
From: Ben Greear @ 2014-05-16 13:26 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath10k, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ath10k: improve vdev map handling.
2014-04-30 18:22 [PATCH] ath10k: improve vdev map handling greearb
2014-05-16 13:18 ` Kalle Valo
@ 2014-05-16 13:37 ` Kalle Valo
2014-05-16 14:01 ` Ben Greear
1 sibling, 1 reply; 7+ messages in thread
From: Kalle Valo @ 2014-05-16 13:37 UTC (permalink / raw)
To: greearb; +Cc: ath10k, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ath10k: improve vdev map handling.
2014-05-16 13:37 ` Kalle Valo
@ 2014-05-16 14:01 ` Ben Greear
2014-05-16 14:06 ` Kalle Valo
0 siblings, 1 reply; 7+ messages in thread
From: Ben Greear @ 2014-05-16 14:01 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath10k, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ath10k: improve vdev map handling.
2014-05-16 14:01 ` Ben Greear
@ 2014-05-16 14:06 ` Kalle Valo
2014-05-16 14:11 ` Ben Greear
0 siblings, 1 reply; 7+ messages in thread
From: Kalle Valo @ 2014-05-16 14:06 UTC (permalink / raw)
To: Ben Greear; +Cc: ath10k, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ath10k: improve vdev map handling.
2014-05-16 14:06 ` Kalle Valo
@ 2014-05-16 14:11 ` Ben Greear
0 siblings, 0 replies; 7+ messages in thread
From: Ben Greear @ 2014-05-16 14:11 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath10k, linux-wireless
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-05-16 14:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-30 18:22 [PATCH] ath10k: improve vdev map handling greearb
2014-05-16 13:18 ` Kalle Valo
2014-05-16 13:26 ` Ben Greear
2014-05-16 13:37 ` Kalle Valo
2014-05-16 14:01 ` Ben Greear
2014-05-16 14:06 ` Kalle Valo
2014-05-16 14:11 ` Ben Greear
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).