Re: [PATCHv2] nl/cfg/mac80211: add set_mcast_rate API

Antonio Quartulli <ordex@xxxxxxxxxxxxx> · Mon, 9 Jul 2012 15:18:13 +0200

On Mon, Jul 09, 2012 at 03:05:46PM +0200, Johannes Berg wrote:
> On Mon, 2012-07-09 at 01:51 +0200, Antonio Quartulli wrote:
> 
> > +static int ieee80211_set_mcast_rate(struct wiphy *wiphy, struct net_device *dev,
> > +				    int mcast_rate[IEEE80211_NUM_BANDS])
> > +{
> > +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > +	u32 basic_rates = sdata->vif.bss_conf.basic_rates;
> > +	int i;
> > +
> > +	/* check if the mcast_rates are also in basic_rates */
> > +	for (i = 0; i < IEEE80211_NUM_BANDS; i++)
> > +		if (!(basic_rates & BIT(mcast_rate[i] - 1)))
> > +			return -EINVAL;
> 
> So this is kinda broken. In fact, the whole basic rate thing is broken
> it seems.
> 
> The mcast rate is per band, as it should, since you could find the same
> BSSID on a 5 GHz channel and then jump to that channel if the TSF is
> higher...
> 
> However, the basic rates aren't, which is wrong: the basic rates bitmap
> could be 1,2,6,9. If the driver is like most drivers, that translates to
> a bitmap of 0x33. But 0x33, for most drivers, if applied to the 5 GHz
> rates means 6,9,24,36. See why this is broken? A rate bitmap can't be
> siwtched around between bands and still make any sense.

I see, I wrongly thought that nl80211_parse_mcast_rate() was checking if the
provided mcast_rate belongs to the basic_rate set of the band which we are now.

But that's wrong! nl80211_parse_mcast_rate() only checks if the provided
mcast_rate exists somewhere...

> 
> Oh, also, I'm not sure why you do BIT(... -1), but that's unrelated.
> What kind of value is the mcast rate? A rate index, or a number?
> 

it's an index and actually it's the index +1, as reported in mac80211.h:

 * @mcast_rate: per-band multicast rate index + 1 (0: disabled)

> johannes

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara