| Message ID | 1341791503-12542-1-git-send-email-ordex@autistici.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. 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? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-07-09 at 15:05 +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. Ok actually maybe it's not broken. We adopt the basic rates from the network we join, I guess, and the original basic rates specified from userspace are only used when we actually *create* the network? Still though, comparing the bits for all bands against the same bitmap like you did in the above code is just wrong :-) Maybe we shouldn't bother at all. If we'd join another network, we might have to downgrade the mcast rate, but then if some peer leaves or we join another one ... it gets messy quickly. Since all of this is probably for special cases anyway, maybe we should just not check the mcast rate and if you set a stupid one you don't get packets through to all peers? Could be hard to debug though ... johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Mon, 2012-07-09 at 15:18 +0200, Antonio Quartulli wrote: > > 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. Well, ok, don't get confused -- there's no basic rate set "of the band", it's a BSS property. > But that's wrong! nl80211_parse_mcast_rate() only checks if the provided > mcast_rate exists somewhere... It checks that each rate exists in the band that it's for. > > 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) also in the cfg80211 API? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 09, 2012 at 03:12:31PM +0200, Johannes Berg wrote: > On Mon, 2012-07-09 at 15:05 +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. > > Ok actually maybe it's not broken. We adopt the basic rates from the > network we join, I guess, and the original basic rates specified from > userspace are only used when we actually *create* the network? mhmm.. I have to check > > Still though, comparing the bits for all bands against the same bitmap > like you did in the above code is just wrong :-) > right, I should select the BAND which we are using and check that index only. > Maybe we shouldn't bother at all. If we'd join another network, we might > have to downgrade the mcast rate, but then if some peer leaves or we > join another one ... it gets messy quickly. > it's adhoc! :-) > Since all of this is probably for special cases anyway, maybe we should > just not check the mcast rate and if you set a stupid one you don't get > packets through to all peers? Could be hard to debug though ... definitely hard. we should keep the intersection of all the rates we have seen and chose the one set by the user if possible or the highest mandatory we have in our set. Sounds too easy :-)
On Mon, Jul 09, 2012 at 03:19:59PM +0200, Johannes Berg wrote: > On Mon, 2012-07-09 at 15:18 +0200, Antonio Quartulli wrote: > > > > 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. > > Well, ok, don't get confused -- there's no basic rate set "of the band", > it's a BSS property. > > > But that's wrong! nl80211_parse_mcast_rate() only checks if the provided > > mcast_rate exists somewhere... > > It checks that each rate exists in the band that it's for. > > > > 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) > > also in the cfg80211 API? yes > > johannes
diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h index db961a5..f8c51af 100644 --- a/include/linux/nl80211.h +++ b/include/linux/nl80211.h @@ -708,6 +708,8 @@ enum nl80211_commands { NL80211_CMD_CH_SWITCH_NOTIFY, + NL80211_CMD_SET_MCAST_RATE, + /* add new commands above here */ /* used to define NL80211_CMD_MAX below */ diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 51f67a9..e55448d 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1722,6 +1722,9 @@ struct cfg80211_ops { int (*set_wiphy_params)(struct wiphy *wiphy, u32 changed); + int (*set_mcast_rate)(struct wiphy *wiphy, struct net_device *dev, + int mcast_rate[IEEE80211_NUM_BANDS]); + int (*set_tx_power)(struct wiphy *wiphy, enum nl80211_tx_power_setting type, int mbm); int (*get_tx_power)(struct wiphy *wiphy, int *dbm); diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index ccbe241..b8f907d 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1919,6 +1919,32 @@ static int ieee80211_set_wiphy_params(struct wiphy *wiphy, u32 changed) return 0; } +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; + + switch (sdata->vif.type) { + case NL80211_IFTYPE_ADHOC: + case NL80211_IFTYPE_MESH_POINT: + memcpy(sdata->vif.bss_conf.mcast_rate, mcast_rate, + sizeof(mcast_rate)); + break; + default: + return -EOPNOTSUPP; + } + + return 0; + +} + static int ieee80211_set_tx_power(struct wiphy *wiphy, enum nl80211_tx_power_setting type, int mbm) { @@ -3041,6 +3067,7 @@ struct cfg80211_ops mac80211_config_ops = { .join_ibss = ieee80211_join_ibss, .leave_ibss = ieee80211_leave_ibss, .set_wiphy_params = ieee80211_set_wiphy_params, + .set_mcast_rate = ieee80211_set_mcast_rate, .set_tx_power = ieee80211_set_tx_power, .get_tx_power = ieee80211_get_tx_power, .set_wds_peer = ieee80211_set_wds_peer, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 2a5cdb6..b0630b2 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -1039,6 +1039,7 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags, if (nla_put_u32(msg, i, NL80211_CMD_REGISTER_BEACONS)) goto nla_put_failure; } + CMD(set_mcast_rate, SET_MCAST_RATE); #ifdef CONFIG_NL80211_TESTMODE CMD(testmode_cmd, TESTMODE); @@ -1668,6 +1669,64 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info) return result; } +static bool +nl80211_parse_mcast_rate(struct cfg80211_registered_device *rdev, + int mcast_rate[IEEE80211_NUM_BANDS], + int rateval) +{ + struct wiphy *wiphy = &rdev->wiphy; + bool found = false; + int band, i; + + for (band = 0; band < IEEE80211_NUM_BANDS; band++) { + struct ieee80211_supported_band *sband; + + sband = wiphy->bands[band]; + if (!sband) + continue; + + for (i = 0; i < sband->n_bitrates; i++) { + if (sband->bitrates[i].bitrate == rateval) { + mcast_rate[band] = i + 1; + found = true; + break; + } + } + } + + return found; +} + +static int nl80211_set_mcast_rate(struct sk_buff *skb, struct genl_info *info) +{ + struct cfg80211_registered_device *rdev = info->user_ptr[0]; + struct net_device *dev = info->user_ptr[1]; + struct wireless_dev *wdev = dev->ieee80211_ptr; + u32 mcast_attr; + int mcast_rate[IEEE80211_NUM_BANDS]; + int err; + + if (!info->attrs[NL80211_ATTR_MCAST_RATE]) + return -EINVAL; + + mcast_attr = nla_get_u32(info->attrs[NL80211_ATTR_MCAST_RATE]); + if (!nl80211_parse_mcast_rate(rdev, mcast_rate, mcast_attr)) + return -EINVAL; + + if (wdev->iftype != NL80211_IFTYPE_ADHOC && + wdev->iftype != NL80211_IFTYPE_MESH_POINT) + return -EOPNOTSUPP; + + if (!rdev->ops->set_mcast_rate) + return -EOPNOTSUPP; + + if (!wdev->current_bss) + return -EINVAL; + + err = rdev->ops->set_mcast_rate(&rdev->wiphy, dev, mcast_rate); + + return err; +} static int nl80211_send_iface(struct sk_buff *msg, u32 pid, u32 seq, int flags, struct cfg80211_registered_device *rdev, @@ -5066,34 +5125,6 @@ static int nl80211_disassociate(struct sk_buff *skb, struct genl_info *info) local_state_change); } -static bool -nl80211_parse_mcast_rate(struct cfg80211_registered_device *rdev, - int mcast_rate[IEEE80211_NUM_BANDS], - int rateval) -{ - struct wiphy *wiphy = &rdev->wiphy; - bool found = false; - int band, i; - - for (band = 0; band < IEEE80211_NUM_BANDS; band++) { - struct ieee80211_supported_band *sband; - - sband = wiphy->bands[band]; - if (!sband) - continue; - - for (i = 0; i < sband->n_bitrates; i++) { - if (sband->bitrates[i].bitrate == rateval) { - mcast_rate[band] = i + 1; - found = true; - break; - } - } - } - - return found; -} - static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; @@ -6726,6 +6757,14 @@ static struct genl_ops nl80211_ops[] = { .internal_flags = NL80211_FLAG_NEED_RTNL, }, { + .cmd = NL80211_CMD_SET_MCAST_RATE, + .doit = nl80211_set_mcast_rate, + .policy = nl80211_policy, + .flags = GENL_ADMIN_PERM, + .internal_flags = NL80211_FLAG_NEED_NETDEV_UP | + NL80211_FLAG_NEED_RTNL, + }, + { .cmd = NL80211_CMD_GET_INTERFACE, .doit = nl80211_get_interface, .dumpit = nl80211_dump_interface,
with the current API the multicast rate parameter is specifiable at ibss and mesh join only. With this new API multicast rate can instead be modified whenever needed by the user (only if the VIF type is ADHOC or MESH_POINT). This API is particularly useful in case of use of wpa_supplicant with IBSS/RSN since it does not support mcast_rate as ibss join parameter (this lead to the impossibility of changing the mcast_rate while using IBSS/RSN). Signed-off-by: Antonio Quartulli <ordex@autistici.org> --- v2: removed useless locking, use BIT() instead of 1<< include/linux/nl80211.h | 2 + include/net/cfg80211.h | 3 ++ net/mac80211/cfg.c | 27 ++++++++++++++ net/wireless/nl80211.c | 95 +++++++++++++++++++++++++++++++++-------------- 4 files changed, 99 insertions(+), 28 deletions(-)