Message ID | 1398882179-17100-1-git-send-email-greearb@candelatech.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
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?
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
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?
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> 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.
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
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) {