Re: [PATCHv2 2/2] mac80211: add support for mcs masks

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]

Hello Johannes,

On Mon, Jan 09, 2012 at 02:04:54PM +0100, Johannes Berg wrote:
> On Thu, 2012-01-05 at 20:58 +0100, Simon Wunderlich wrote:
> 
> > +static bool rate_idx_match_mcs_mask(struct ieee80211_tx_rate *rate,
> > +				    u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN])
> > +{
> > +	int i, j;
> > +	int ridx, rbit;
> > +
> > +	ridx = rate->idx / 8;
> > +	rbit = rate->idx % 8;
> > +
> > +	/* sanity check */
> > +	if (ridx < 0 || ridx > IEEE80211_HT_MCS_MASK_LEN)
> > +		return false;
> > +
> > +	/* See whether the selected rate or anything below it is allowed. */
> > +	for (i = ridx; i >= 0; i--) {
> > +		for (j = rbit; j >= 0; j--)
> > +			if (mcs_mask[i] & BIT(j)) {
> > +				rate->idx = i * 8 + j;
> > +				return true;
> > +			}
> > +		rbit = 7;
> > +	}
> > +
> > +	/* Try to find a higher rate that would be allowed */
> > +	ridx = (rate->idx + 1) / 8;
> > +	rbit = (rate->idx + 1) % 8;
> > +
> > +	for (i = ridx; i < IEEE80211_HT_MCS_MASK_LEN; i++) {
> > +		for (j = rbit; j < 8; j++)
> > +			if (mcs_mask[i] & BIT(j)) {
> > +				rate->idx = i * 8 + j;
> > +				return true;
> > +			}
> > +		rbit = 0;
> > +	}
> > +	return false;
> 
> All this logic is pretty complex, maybe we could translate the bitmap to
> an array of unsigned longs and use test_bit() instead of open-coding it
> for 8-bit words?

Mhm, I would rather not add further copies into this fast path function, and
simply casting will result in bad behaviour (alignment, endianess). Generally
changing the mcs_mask to u32 is possible, but then we have inconsistent mcs
fields (rx_mask and others are u8 arrays too) - do we want this? Or am I missing
something obvious right now? :)

Maybe we can simplify it to something like this:

for (i = rate->idx + 1; i < IEEE80211_HT_MCS_MASK_LEN * 8; i++) {
	ridx = i / 8;
	rbit = i % 8;
	if (mcs_mask[ridx] & BIT(rbit))) {
		rate->idx = i;
		return true;
	}
}

What do you think?

Cheers,
	Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]