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

[Ben Greear <greearb@candelatech.com>]

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