[PATCH] cfg80211: restrict AP beacon intervals

[PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Multiple virtual AP interfaces can currently try
to use different beacon intervals, but that just
leads to problems since it won't actually be done
that way by drivers. Return an error in this case
to make sure it won't be done wrong.

Also, ignore attempts to change the DTIM period
or beacon interval during the lifetime of the BSS.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h |    4 ++++
 net/wireless/core.c    |    1 +
 net/wireless/core.h    |    3 +++
 net/wireless/nl80211.c |   40 +++++++++++++++++++++++-----------------
 net/wireless/util.c    |   25 +++++++++++++++++++++++++
 5 files changed, 56 insertions(+), 17 deletions(-)

--- a/include/net/cfg80211.h	2011-05-06 17:40:17.000000000 +0200
+++ b/include/net/cfg80211.h	2011-05-06 17:40:17.000000000 +0200
@@ -1836,6 +1836,8 @@ struct cfg80211_cached_keys;
  * @mgmt_registrations_lock: lock for the list
  * @mtx: mutex used to lock data in this struct
  * @cleanup_work: work struct used for cleanup that can't be done directly
+ * @beacon_interval: beacon interval used on this device for transmitting
+ *	beacons, 0 when not valid
  */
 struct wireless_dev {
 	struct wiphy *wiphy;
@@ -1876,6 +1878,8 @@ struct wireless_dev {
 	bool ps;
 	int ps_timeout;
 
+	int beacon_interval;
+
 #ifdef CONFIG_CFG80211_WEXT
 	/* wext data */
 	struct {
--- a/net/wireless/core.h	2011-05-06 17:40:17.000000000 +0200
+++ b/net/wireless/core.h	2011-05-06 17:40:17.000000000 +0200
@@ -426,6 +426,9 @@ int cfg80211_set_freq(struct cfg80211_re
 
 u16 cfg80211_calculate_bitrate(struct rate_info *rate);
 
+int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
+				 u32 beacon_int);
+
 #ifdef CONFIG_CFG80211_DEVELOPER_WARNINGS
 #define CFG80211_DEV_WARN_ON(cond)	WARN_ON(cond)
 #else
--- a/net/wireless/util.c	2011-05-06 17:40:17.000000000 +0200
+++ b/net/wireless/util.c	2011-05-06 17:44:40.000000000 +0200
@@ -896,3 +896,28 @@ u16 cfg80211_calculate_bitrate(struct ra
 	/* do NOT round down here */
 	return (bitrate + 50000) / 100000;
 }
+
+int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
+				 u32 beacon_int)
+{
+	struct wireless_dev *wdev;
+	int res = 0;
+
+	if (!beacon_int)
+		return -EINVAL;
+
+	mutex_lock(&rdev->devlist_mtx);
+
+	list_for_each_entry(wdev, &rdev->netdev_list, list) {
+		if (!wdev->beacon_interval)
+			continue;
+		if (wdev->beacon_interval != beacon_int) {
+			res = -EINVAL;
+			break;
+		}
+	}
+
+	mutex_unlock(&rdev->devlist_mtx);
+
+	return res;
+}
--- a/net/wireless/nl80211.c	2011-05-06 17:40:17.000000000 +0200
+++ b/net/wireless/nl80211.c	2011-05-06 17:40:17.000000000 +0200
@@ -1871,8 +1871,9 @@ static int nl80211_addset_beacon(struct
 		    struct beacon_parameters *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;
 	struct beacon_parameters params;
-	int haveinfo = 0;
+	int haveinfo = 0, err;
 
 	if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_BEACON_TAIL]))
 		return -EINVAL;
@@ -1881,6 +1882,8 @@ static int nl80211_addset_beacon(struct
 	    dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
 		return -EOPNOTSUPP;
 
+	memset(&params, 0, sizeof(params));
+
 	switch (info->genlhdr->cmd) {
 	case NL80211_CMD_NEW_BEACON:
 		/* these are required for NEW_BEACON */
@@ -1889,6 +1892,15 @@ static int nl80211_addset_beacon(struct
 		    !info->attrs[NL80211_ATTR_BEACON_HEAD])
 			return -EINVAL;
 
+		params.interval =
+			nla_get_u32(info->attrs[NL80211_ATTR_BEACON_INTERVAL]);
+		params.dtim_period =
+			nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);
+
+		err = cfg80211_validate_beacon_int(rdev, params.interval);
+		if (err)
+			return err;
+
 		call = rdev->ops->add_beacon;
 		break;
 	case NL80211_CMD_SET_BEACON:
@@ -1902,20 +1914,6 @@ static int nl80211_addset_beacon(struct
 	if (!call)
 		return -EOPNOTSUPP;
 
-	memset(&params, 0, sizeof(params));
-
-	if (info->attrs[NL80211_ATTR_BEACON_INTERVAL]) {
-		params.interval =
-		    nla_get_u32(info->attrs[NL80211_ATTR_BEACON_INTERVAL]);
-		haveinfo = 1;
-	}
-
-	if (info->attrs[NL80211_ATTR_DTIM_PERIOD]) {
-		params.dtim_period =
-		    nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);
-		haveinfo = 1;
-	}
-
 	if (info->attrs[NL80211_ATTR_BEACON_HEAD]) {
 		params.head = nla_data(info->attrs[NL80211_ATTR_BEACON_HEAD]);
 		params.head_len =
@@ -1933,13 +1931,18 @@ static int nl80211_addset_beacon(struct
 	if (!haveinfo)
 		return -EINVAL;
 
-	return call(&rdev->wiphy, dev, &params);
+	err = call(&rdev->wiphy, dev, &params);
+	if (!err && params.interval)
+		wdev->beacon_interval = params.interval;
+	return err;
 }
 
 static int nl80211_del_beacon(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;
+	int err;
 
 	if (!rdev->ops->del_beacon)
 		return -EOPNOTSUPP;
@@ -1948,7 +1951,10 @@ static int nl80211_del_beacon(struct sk_
 	    dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
 		return -EOPNOTSUPP;
 
-	return rdev->ops->del_beacon(&rdev->wiphy, dev);
+	err = rdev->ops->del_beacon(&rdev->wiphy, dev);
+	if (!err)
+		wdev->beacon_interval = 0;
+	return err;
 }
 
 static const struct nla_policy sta_flags_policy[NL80211_STA_FLAG_MAX + 1] = {
--- a/net/wireless/core.c	2011-05-06 17:41:05.000000000 +0200
+++ b/net/wireless/core.c	2011-05-06 17:41:25.000000000 +0200
@@ -777,6 +777,7 @@ static int cfg80211_netdev_notifier_call
 		default:
 			break;
 		}
+		wdev->beacon_interval = 0;
 		break;
 	case NETDEV_DOWN:
 		dev_hold(dev);

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Ben Greear]

On 05/09/2011 09:41 AM, Johannes Berg wrote:
> From: Johannes Berg<johannes.berg@intel.com>
>
> Multiple virtual AP interfaces can currently try
> to use different beacon intervals, but that just
> leads to problems since it won't actually be done
> that way by drivers. Return an error in this case
> to make sure it won't be done wrong.

I think there is no problem with having different beacon
intervals, as long as they are all a multiple of
the smallest interval and the driver does things properly.

I'm not sure ath9k or ath5k currently supports this properly,
but there was a patch floating around for a while that did
this for ath9k I think...

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Mon, 2011-05-09 at 09:46 -0700, Ben Greear wrote:
> On 05/09/2011 09:41 AM, Johannes Berg wrote:
> > From: Johannes Berg<johannes.berg@intel.com>
> >
> > Multiple virtual AP interfaces can currently try
> > to use different beacon intervals, but that just
> > leads to problems since it won't actually be done
> > that way by drivers. Return an error in this case
> > to make sure it won't be done wrong.
> 
> I think there is no problem with having different beacon
> intervals, as long as they are all a multiple of
> the smallest interval and the driver does things properly.
> 
> I'm not sure ath9k or ath5k currently supports this properly,
> but there was a patch floating around for a while that did
> this for ath9k I think...

Yes, in theory that's possible, but apparently no driver actually did
this correctly. Also, it didn't seem like anyone really cares, and we
need to enforce some restrictions because otherwise drivers will end up
doing it wrong, and you'll end up having a beacon interval of 200 while
advertising 150 for example, which will totally throw off powersaving
clients.

If you really care greatly about having different beacon intervals (and
I don't see why you would?) then maybe you can think how we can enforce
and advertise that to userspace. For now, I'm more comfortable just
restricting it.

johannes

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Ben Greear]

On 05/09/2011 09:49 AM, Johannes Berg wrote:
> On Mon, 2011-05-09 at 09:46 -0700, Ben Greear wrote:
>> On 05/09/2011 09:41 AM, Johannes Berg wrote:
>>> From: Johannes Berg<johannes.berg@intel.com>
>>>
>>> Multiple virtual AP interfaces can currently try
>>> to use different beacon intervals, but that just
>>> leads to problems since it won't actually be done
>>> that way by drivers. Return an error in this case
>>> to make sure it won't be done wrong.
>>
>> I think there is no problem with having different beacon
>> intervals, as long as they are all a multiple of
>> the smallest interval and the driver does things properly.
>>
>> I'm not sure ath9k or ath5k currently supports this properly,
>> but there was a patch floating around for a while that did
>> this for ath9k I think...
>
> Yes, in theory that's possible, but apparently no driver actually did
> this correctly. Also, it didn't seem like anyone really cares, and we
> need to enforce some restrictions because otherwise drivers will end up
> doing it wrong, and you'll end up having a beacon interval of 200 while
> advertising 150 for example, which will totally throw off powersaving
> clients.
>
> If you really care greatly about having different beacon intervals (and
> I don't see why you would?) then maybe you can think how we can enforce
> and advertise that to userspace. For now, I'm more comfortable just
> restricting it.

I guess we could add a flag to the driver when it supports it properly
and modify your logic to check for even multiples instead of just ==
if that flag is set?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Mon, 2011-05-09 at 09:53 -0700, Ben Greear wrote:

> >> I think there is no problem with having different beacon
> >> intervals, as long as they are all a multiple of
> >> the smallest interval and the driver does things properly.
> >>
> >> I'm not sure ath9k or ath5k currently supports this properly,
> >> but there was a patch floating around for a while that did
> >> this for ath9k I think...
> >
> > Yes, in theory that's possible, but apparently no driver actually did
> > this correctly. Also, it didn't seem like anyone really cares, and we
> > need to enforce some restrictions because otherwise drivers will end up
> > doing it wrong, and you'll end up having a beacon interval of 200 while
> > advertising 150 for example, which will totally throw off powersaving
> > clients.
> >
> > If you really care greatly about having different beacon intervals (and
> > I don't see why you would?) then maybe you can think how we can enforce
> > and advertise that to userspace. For now, I'm more comfortable just
> > restricting it.
> 
> I guess we could add a flag to the driver when it supports it properly
> and modify your logic to check for even multiples instead of just ==
> if that flag is set?

Yes, but it's not just a flag, you also need the smallest divisor that
the driver needs, for example at least 50 TU. Also, when multiple
channels are used (not right now of course) that might factor into the
calculations of when which beacon is sent.

johannes

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Steve Brown]

On Mon, 2011-05-09 at 18:49 +0200, Johannes Berg wrote:
> On Mon, 2011-05-09 at 09:46 -0700, Ben Greear wrote:
> > On 05/09/2011 09:41 AM, Johannes Berg wrote:
> > > From: Johannes Berg<johannes.berg@intel.com>
> > >
> > > Multiple virtual AP interfaces can currently try
> > > to use different beacon intervals, but that just
> > > leads to problems since it won't actually be done
> > > that way by drivers. Return an error in this case
> > > to make sure it won't be done wrong.
> > 
> > I think there is no problem with having different beacon
> > intervals, as long as they are all a multiple of
> > the smallest interval and the driver does things properly.
> > 
> > I'm not sure ath9k or ath5k currently supports this properly,
> > but there was a patch floating around for a while that did
> > this for ath9k I think...
> 
> If you really care greatly about having different beacon intervals (and
> I don't see why you would?) then maybe you can think how we can enforce
> and advertise that to userspace. For now, I'm more comfortable just
> restricting it.
> 
> johannes

I posted a patch for different beacon intervals to the ath9k list last
Jan. This was to allow both a mesh (interval 1000) and ap (interval 100)
vif on the same radio. This combination seems useful, at least for me.

Steve

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Mon, 2011-05-09 at 13:20 -0400, Steve Brown wrote:

> > 
> > If you really care greatly about having different beacon intervals (and
> > I don't see why you would?) then maybe you can think how we can enforce
> > and advertise that to userspace. For now, I'm more comfortable just
> > restricting it.
> > 
> > johannes
> 
> I posted a patch for different beacon intervals to the ath9k list last
> Jan. This was to allow both a mesh (interval 1000) and ap (interval 100)
> vif on the same radio. This combination seems useful, at least for me.

Yeah that makes some sense. Note that this patch doesn't (yet) enforce
any restrictions on mesh anyway though.

johannes

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Ben Greear]

On 05/09/2011 10:20 AM, Steve Brown wrote:
> On Mon, 2011-05-09 at 18:49 +0200, Johannes Berg wrote:
>> On Mon, 2011-05-09 at 09:46 -0700, Ben Greear wrote:
>>> On 05/09/2011 09:41 AM, Johannes Berg wrote:
>>>> From: Johannes Berg<johannes.berg@intel.com>
>>>>
>>>> Multiple virtual AP interfaces can currently try
>>>> to use different beacon intervals, but that just
>>>> leads to problems since it won't actually be done
>>>> that way by drivers. Return an error in this case
>>>> to make sure it won't be done wrong.
>>>
>>> I think there is no problem with having different beacon
>>> intervals, as long as they are all a multiple of
>>> the smallest interval and the driver does things properly.
>>>
>>> I'm not sure ath9k or ath5k currently supports this properly,
>>> but there was a patch floating around for a while that did
>>> this for ath9k I think...
>>
>> If you really care greatly about having different beacon intervals (and
>> I don't see why you would?) then maybe you can think how we can enforce
>> and advertise that to userspace. For now, I'm more comfortable just
>> restricting it.
>>
>> johannes
>
> I posted a patch for different beacon intervals to the ath9k list last
> Jan. This was to allow both a mesh (interval 1000) and ap (interval 100)
> vif on the same radio. This combination seems useful, at least for me.

Could you re-post this against latest wireless-testing?  I should
be able to do some testing on this and maybe we can get it pushed
upstream...

Ben

>
> Steve


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Björn Smedman]

On Mon, May 9, 2011 at 6:49 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2011-05-09 at 09:46 -0700, Ben Greear wrote:
>> On 05/09/2011 09:41 AM, Johannes Berg wrote:
>> > From: Johannes Berg<johannes.berg@intel.com>
>> >
>> > Multiple virtual AP interfaces can currently try
>> > to use different beacon intervals, but that just
>> > leads to problems since it won't actually be done
>> > that way by drivers. Return an error in this case
>> > to make sure it won't be done wrong.
>>
>> I think there is no problem with having different beacon
>> intervals, as long as they are all a multiple of
>> the smallest interval and the driver does things properly.
>>
>> I'm not sure ath9k or ath5k currently supports this properly,
>> but there was a patch floating around for a while that did
>> this for ath9k I think...
>
> Yes, in theory that's possible, but apparently no driver actually did
> this correctly. Also, it didn't seem like anyone really cares, and we
> need to enforce some restrictions because otherwise drivers will end up
> doing it wrong, and you'll end up having a beacon interval of 200 while
> advertising 150 for example, which will totally throw off powersaving
> clients.

I'm very interested in having multiple AP vifs with different beacon
intervals. If we're going to just fail anyway in this case can't we do
that from the drivers instead? I would also prefer that from an
aesthetic point of view, instead of having broken logic in the drivers
"protected" by extra verification in cfg80211.

Best regards,

Björn

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Wed, 2011-05-11 at 17:58 +0200, Björn Smedman wrote:

> > Yes, in theory that's possible, but apparently no driver actually did
> > this correctly. Also, it didn't seem like anyone really cares, and we
> > need to enforce some restrictions because otherwise drivers will end up
> > doing it wrong, and you'll end up having a beacon interval of 200 while
> > advertising 150 for example, which will totally throw off powersaving
> > clients.
> 
> I'm very interested in having multiple AP vifs with different beacon
> intervals. If we're going to just fail anyway in this case can't we do
> that from the drivers instead? I would also prefer that from an
> aesthetic point of view, instead of having broken logic in the drivers
> "protected" by extra verification in cfg80211.

We can't fail from the drivers, they don't have a failure path.

johannes

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Ben Greear]

On 05/11/2011 09:48 AM, Johannes Berg wrote:
> On Wed, 2011-05-11 at 17:58 +0200, Björn Smedman wrote:
>
>>> Yes, in theory that's possible, but apparently no driver actually did
>>> this correctly. Also, it didn't seem like anyone really cares, and we
>>> need to enforce some restrictions because otherwise drivers will end up
>>> doing it wrong, and you'll end up having a beacon interval of 200 while
>>> advertising 150 for example, which will totally throw off powersaving
>>> clients.
>>
>> I'm very interested in having multiple AP vifs with different beacon
>> intervals. If we're going to just fail anyway in this case can't we do
>> that from the drivers instead? I would also prefer that from an
>> aesthetic point of view, instead of having broken logic in the drivers
>> "protected" by extra verification in cfg80211.
>
> We can't fail from the drivers, they don't have a failure path.

Maybe we could treat the beacon interval setting as a requested
value, and give the caller some way to know the actual value
that is used by the hardware?

Thanks,
Ben


>
> 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


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Wed, 2011-05-11 at 10:10 -0700, Ben Greear wrote:
> On 05/11/2011 09:48 AM, Johannes Berg wrote:
> > On Wed, 2011-05-11 at 17:58 +0200, Björn Smedman wrote:
> >
> >>> Yes, in theory that's possible, but apparently no driver actually did
> >>> this correctly. Also, it didn't seem like anyone really cares, and we
> >>> need to enforce some restrictions because otherwise drivers will end up
> >>> doing it wrong, and you'll end up having a beacon interval of 200 while
> >>> advertising 150 for example, which will totally throw off powersaving
> >>> clients.
> >>
> >> I'm very interested in having multiple AP vifs with different beacon
> >> intervals. If we're going to just fail anyway in this case can't we do
> >> that from the drivers instead? I would also prefer that from an
> >> aesthetic point of view, instead of having broken logic in the drivers
> >> "protected" by extra verification in cfg80211.
> >
> > We can't fail from the drivers, they don't have a failure path.
> 
> Maybe we could treat the beacon interval setting as a requested
> value, and give the caller some way to know the actual value
> that is used by the hardware?

No, not really, the value needs to be in the beacon data.

johannes

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Björn Smedman]

2011/5/11 Johannes Berg <johannes@sipsolutions.net>:
> On Wed, 2011-05-11 at 17:58 +0200, Björn Smedman wrote:
>
>> > Yes, in theory that's possible, but apparently no driver actually did
>> > this correctly. Also, it didn't seem like anyone really cares, and we
>> > need to enforce some restrictions because otherwise drivers will end up
>> > doing it wrong, and you'll end up having a beacon interval of 200 while
>> > advertising 150 for example, which will totally throw off powersaving
>> > clients.
>>
>> I'm very interested in having multiple AP vifs with different beacon
>> intervals. If we're going to just fail anyway in this case can't we do
>> that from the drivers instead? I would also prefer that from an
>> aesthetic point of view, instead of having broken logic in the drivers
>> "protected" by extra verification in cfg80211.
>
> We can't fail from the drivers, they don't have a failure path.

Wouldn't it be best to add a failure path? Based a quick look it seems
a little complicated but not unmanageable... Or what do you think?

Is there anything else where the driver may not support a certain
combination of bss configurations? From the top of my head I can think
of mac address. That should fail as well depending on HW capability,
right? That seems to be another code path but the same problem (no
failure path).

/Björn

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Wed, 2011-05-11 at 23:39 +0200, Björn Smedman wrote:
> 2011/5/11 Johannes Berg <johannes@sipsolutions.net>:
> > On Wed, 2011-05-11 at 17:58 +0200, Björn Smedman wrote:
> >
> >> > Yes, in theory that's possible, but apparently no driver actually did
> >> > this correctly. Also, it didn't seem like anyone really cares, and we
> >> > need to enforce some restrictions because otherwise drivers will end up
> >> > doing it wrong, and you'll end up having a beacon interval of 200 while
> >> > advertising 150 for example, which will totally throw off powersaving
> >> > clients.
> >>
> >> I'm very interested in having multiple AP vifs with different beacon
> >> intervals. If we're going to just fail anyway in this case can't we do
> >> that from the drivers instead? I would also prefer that from an
> >> aesthetic point of view, instead of having broken logic in the drivers
> >> "protected" by extra verification in cfg80211.
> >
> > We can't fail from the drivers, they don't have a failure path.
> 
> Wouldn't it be best to add a failure path? Based a quick look it seems
> a little complicated but not unmanageable... Or what do you think?

It's not overly complicated, but I don't like it anyway, that just opens
the door to the drivers failing all kinds of things and to userspace
that'll look random.

> Is there anything else where the driver may not support a certain
> combination of bss configurations? From the top of my head I can think
> of mac address. That should fail as well depending on HW capability,
> right? That seems to be another code path but the same problem (no
> failure path).

That has a failure path, and recently I posted a patch to advertise the
interface type combinations. I don't think MAC addresses impose any sort
of restriction.


Bottom line is this: We currently don't have a driver that handles this
correctly. Therefore, the patch is absolutely correct. If somebody
enhances drivers, they're free to also enhance the checking code, and
(hopefully) include some advertising of what is possible. Currently,
however, the best assumption is that it's not possible, and we should
encode that assumption somewhere in the userspace API.

johannes

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Björn Smedman]

2011/5/12 Johannes Berg <johannes@sipsolutions.net>:
> On Wed, 2011-05-11 at 23:39 +0200, Björn Smedman wrote:
>> 2011/5/11 Johannes Berg <johannes@sipsolutions.net>:
>> > On Wed, 2011-05-11 at 17:58 +0200, Björn Smedman wrote:
>> >> I'm very interested in having multiple AP vifs with different beacon
>> >> intervals. If we're going to just fail anyway in this case can't we do
>> >> that from the drivers instead? I would also prefer that from an
>> >> aesthetic point of view, instead of having broken logic in the drivers
>> >> "protected" by extra verification in cfg80211.
>> >
>> > We can't fail from the drivers, they don't have a failure path.
>>
>> Wouldn't it be best to add a failure path? Based a quick look it seems
>> a little complicated but not unmanageable... Or what do you think?
>
> It's not overly complicated, but I don't like it anyway, that just opens
> the door to the drivers failing all kinds of things and to userspace
> that'll look random.
>
>> Is there anything else where the driver may not support a certain
>> combination of bss configurations? From the top of my head I can think
>> of mac address. That should fail as well depending on HW capability,
>> right? That seems to be another code path but the same problem (no
>> failure path).
>
> That has a failure path, and recently I posted a patch to advertise the
> interface type combinations. I don't think MAC addresses impose any sort
> of restriction.

I think at least some rt2x00 hw may not be able to handle all
combinations of multi-vif MAC addresses.

If I understand correctly ath9k will happily configure the hardware to
ack all BSSIDs if multi-vif MAC addresses are poorly chosen. I don't
know if that's a reason to fail but it wouldn't be good, right?

Another case where cross-vif constraints are difficult to handle is
BSS_CHANGED_ERP_SLOT / BSS_CHANGED_ERP_PREAMBLE. Again, I'm not sure
failing is the right way to handle this but at least ath9k seems
incapable of having separate slot times / preambles for separate
vifs...

> Bottom line is this: We currently don't have a driver that handles this
> correctly. Therefore, the patch is absolutely correct. If somebody
> enhances drivers, they're free to also enhance the checking code, and
> (hopefully) include some advertising of what is possible. Currently,
> however, the best assumption is that it's not possible, and we should
> encode that assumption somewhere in the userspace API.

Sounds reasonable. Thanks for explaining.

/Björn

Re: [PATCH] cfg80211: restrict AP beacon intervals

[Johannes Berg]

On Thu, 2011-05-12 at 20:08 +0200, Björn Smedman wrote:

> > That has a failure path, and recently I posted a patch to advertise the
> > interface type combinations. I don't think MAC addresses impose any sort
> > of restriction.
> 
> I think at least some rt2x00 hw may not be able to handle all
> combinations of multi-vif MAC addresses.
> 
> If I understand correctly ath9k will happily configure the hardware to
> ack all BSSIDs if multi-vif MAC addresses are poorly chosen. I don't
> know if that's a reason to fail but it wouldn't be good, right?

That'd be possible I guess if you configure them so their XOR is all
ones, but it doesn't matter -- you can fail that right now. Also, if the
user configures it like that maybe that's their fault?

> Another case where cross-vif constraints are difficult to handle is
> BSS_CHANGED_ERP_SLOT / BSS_CHANGED_ERP_PREAMBLE. Again, I'm not sure
> failing is the right way to handle this but at least ath9k seems
> incapable of having separate slot times / preambles for separate
> vifs...

That's hard to believe since it's rate control stuff?

johannes