[RFC] nl80211: don't require netdev UP for wdev

[RFC] nl80211: don't require netdev UP for wdev

[Thomas Pedersen]

It seems as long as we have a netdev, a wdev is available as well.
Remove the restriction that netdev must be up before a wdev can be
obtained in nl80211, or changing channels in cfg80211.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>

---
Hi list,

This was encountered while implementing an interface for setting
BSSBasicRateSet in mesh. The wdev->channel is needed in nl80211.c for
rate verification, but prior to this patch not available.  This doesn't
seem right since the following sequence of commands would fail:

iw wlan0 set type mp
iw wlan0 set channel n
ip link set wlan0 up
iw wlan0 mesh join meshid basic-rate 1,2

Also, 'iw set channel' is met with an -EBUSY if doing this after the
link is up for fixed channel modes (mesh) anyway.

Comments? Any idea why this was required initially?

Thanks,
Thomas

 net/wireless/chan.c    |    6 +-----
 net/wireless/nl80211.c |    3 +--
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 2fcfe09..2ae6019 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -88,13 +88,9 @@ int cfg80211_set_freq(struct cfg80211_registered_device *rdev,
 	if (wdev && wdev->iftype == NL80211_IFTYPE_MONITOR)
 		wdev = NULL;
 
-	if (wdev) {
+	if (wdev)
 		ASSERT_WDEV_LOCK(wdev);
 
-		if (!netif_running(wdev->netdev))
-			return -ENETDOWN;
-	}
-
 	if (!rdev->ops->set_channel)
 		return -EOPNOTSUPP;
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 140c1d2..29e5703 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1257,8 +1257,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		result = 0;
 
 		mutex_lock(&rdev->mtx);
-	} else if (netif_running(netdev) &&
-		   nl80211_can_set_dev_channel(netdev->ieee80211_ptr))
+	} else if (nl80211_can_set_dev_channel(netdev->ieee80211_ptr))
 		wdev = netdev->ieee80211_ptr;
 	else
 		wdev = NULL;
-- 
1.7.5.4

Re: [RFC] nl80211: don't require netdev UP for wdev

[Johannes Berg]

On Thu, 2012-04-26 at 15:28 -0700, Thomas Pedersen wrote:
> It seems as long as we have a netdev, a wdev is available as well.
> Remove the restriction that netdev must be up before a wdev can be
> obtained in nl80211, or changing channels in cfg80211.
> 
> Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
> 
> ---
> Hi list,
> 
> This was encountered while implementing an interface for setting
> BSSBasicRateSet in mesh. The wdev->channel is needed in nl80211.c for
> rate verification, but prior to this patch not available.  This doesn't
> seem right since the following sequence of commands would fail:
> 
> iw wlan0 set type mp
> iw wlan0 set channel n
> ip link set wlan0 up
> iw wlan0 mesh join meshid basic-rate 1,2
> 
> Also, 'iw set channel' is met with an -EBUSY if doing this after the
> link is up for fixed channel modes (mesh) anyway.
> 
> Comments? Any idea why this was required initially?


I don't think this patch is correct -- mac80211 will get updated even if
the device isn't even started which will likely cause trouble. Even if
not though, this all doesn't match any kind of multi-channel concept. We
treat the channel as part of the temporary setup, e.g. part of the
association. AP and mesh are the only ones that are different today I
think.

Overall, I don't think setting the channel & doing mesh setup separately
is really a good API.

>From mac80211's (and other drivers if they existed) POV the channel
should be given with the mesh_join command, like for IBSS.

Now, obviously, requiring userspace to do that would be an API change.
We probably don't want that, so I would suggest to change cfg80211 to
track the channel and then pass it to join_mesh as one of the mesh
parameters. This could be made work even when somebody attempts to set
the channel before the interface is up, but we'd have to be careful
about interface type changes.

We should do the same for AP mode as well, since the channel really
becomes relevant only upon start_ap(), before that there's no real
concept of a channel since you don't use it yet anyway.

johannes

Re: [RFC] nl80211: don't require netdev UP for wdev

[Michal Kazior]

Johannes Berg wrote:
> On Thu, 2012-04-26 at 15:28 -0700, Thomas Pedersen wrote:
>> iw wlan0 set type mp
>> iw wlan0 set channel n
>> ip link set wlan0 up
>> iw wlan0 mesh join meshid basic-rate 1,2
>>
>> Also, 'iw set channel' is met with an -EBUSY if doing this after the
>> link is up for fixed channel modes (mesh) anyway.
>>
>> Comments? Any idea why this was required initially?
>
>
> I don't think this patch is correct -- mac80211 will get updated even if
> the device isn't even started which will likely cause trouble. Even if
> not though, this all doesn't match any kind of multi-channel concept. We
> treat the channel as part of the temporary setup, e.g. part of the
> association. AP and mesh are the only ones that are different today I
> think.

What about monitor mode?


> Overall, I don't think setting the channel&  doing mesh setup separately
> is really a good API.
>
>  From mac80211's (and other drivers if they existed) POV the channel
> should be given with the mesh_join command, like for IBSS.
>
> Now, obviously, requiring userspace to do that would be an API change.
> We probably don't want that, so I would suggest to change cfg80211 to
> track the channel and then pass it to join_mesh as one of the mesh
> parameters. This could be made work even when somebody attempts to set
> the channel before the interface is up, but we'd have to be careful
> about interface type changes.
>
> We should do the same for AP mode as well, since the channel really
> becomes relevant only upon start_ap(), before that there's no real
> concept of a channel since you don't use it yet anyway.

Maybe we should do the same with monitor interface type, i.e. introduce 
start_monitor() and have it pass the channel.

But then I'm thinking we shouldn't really be changing interface types 
explicitly, since they would be set implicitly by start_ap() and such. 
We could then use the NL80211_IFTYPE_UNSPECIFIED when we're not associated.


-- 
Pozdrawiam / Best regards, Michal Kazior.

Re: [RFC] nl80211: don't require netdev UP for wdev

[Johannes Berg]

On Wed, 2012-05-09 at 11:41 +0200, Michal Kazior wrote:
> Johannes Berg wrote:

> > I don't think this patch is correct -- mac80211 will get updated even if
> > the device isn't even started which will likely cause trouble. Even if
> > not though, this all doesn't match any kind of multi-channel concept. We
> > treat the channel as part of the temporary setup, e.g. part of the
> > association. AP and mesh are the only ones that are different today I
> > think.
> 
> What about monitor mode?

It's ... special and confusing, unfortunately.
Basically, it's handled in mac80211 today, it rejects monitor channel
setting unless it's otherwise idle, and overrides it freely if other
requests come in.


> > We should do the same for AP mode as well, since the channel really
> > becomes relevant only upon start_ap(), before that there's no real
> > concept of a channel since you don't use it yet anyway.
> 
> Maybe we should do the same with monitor interface type, i.e. introduce 
> start_monitor() and have it pass the channel.

No, that's the special case that doesn't work that way -- monitor mode
interfaces are also great for debugging when they're just concurrently
running with other interfaces and don't have their own channel setting.

> But then I'm thinking we shouldn't really be changing interface types 
> explicitly, since they would be set implicitly by start_ap() and such. 
> We could then use the NL80211_IFTYPE_UNSPECIFIED when we're not associated.

>From some point of view, it should work that way, yes.

Given how entrenched interface types are we definitely can't get rid of
them though, and they're still needed to do concurrency management etc.
But changing everything to have explicit start/stop operations makes
on-the-fly interface type changes much easier.

johannes

Re: [RFC] nl80211: don't require netdev UP for wdev

[Michal Kazior]

Johannes Berg wrote:
>>> We should do the same for AP mode as well, since the channel really
>>> becomes relevant only upon start_ap(), before that there's no real
>>> concept of a channel since you don't use it yet anyway.
>>
>> Maybe we should do the same with monitor interface type, i.e. introduce
>> start_monitor() and have it pass the channel.
>
> No, that's the special case that doesn't work that way -- monitor mode
> interfaces are also great for debugging when they're just concurrently
> running with other interfaces and don't have their own channel setting.

We can make monitor interface to be a slave to other non-monitor 
interface by passing an extra parameter in start_monitor(). We could 
then have such a monitor interface follow channel changes of a given 
non-monitor interface (or rather it wouldn't be using a channel on it's 
own).


>> But then I'm thinking we shouldn't really be changing interface types
>> explicitly, since they would be set implicitly by start_ap() and such.
>> We could then use the NL80211_IFTYPE_UNSPECIFIED when we're not associated.
>
>  From some point of view, it should work that way, yes.
>
> Given how entrenched interface types are we definitely can't get rid of
> them though, and they're still needed to do concurrency management etc.

Well, I don't think we can get completely rid of them, too. Or do you 
mean something else?


-- 
Pozdrawiam / Best regards, Michal Kazior.

Re: [RFC] nl80211: don't require netdev UP for wdev

[Johannes Berg]

On Wed, 2012-05-09 at 12:16 +0200, Michal Kazior wrote:

> > No, that's the special case that doesn't work that way -- monitor mode
> > interfaces are also great for debugging when they're just concurrently
> > running with other interfaces and don't have their own channel setting.
> 
> We can make monitor interface to be a slave to other non-monitor 
> interface by passing an extra parameter in start_monitor(). We could 
> then have such a monitor interface follow channel changes of a given 
> non-monitor interface (or rather it wouldn't be using a channel on it's 
> own).

That's not how it works though, a monitor interface will simply report
all frames mac80211 receives, globally.

> >> But then I'm thinking we shouldn't really be changing interface types
> >> explicitly, since they would be set implicitly by start_ap() and such.
> >> We could then use the NL80211_IFTYPE_UNSPECIFIED when we're not associated.
> >
> >  From some point of view, it should work that way, yes.
> >
> > Given how entrenched interface types are we definitely can't get rid of
> > them though, and they're still needed to do concurrency management etc.
> 
> Well, I don't think we can get completely rid of them, too. Or do you 
> mean something else?

We can't even implicitly change them due to concurrency management etc.

johannes

Re: [RFC] nl80211: don't require netdev UP for wdev

[Thomas Pedersen]

Hi Johannes,

Thanks for reviewing.

On Wed, May 9, 2012 at 2:15 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2012-04-26 at 15:28 -0700, Thomas Pedersen wrote:
>> It seems as long as we have a netdev, a wdev is available as well.
>> Remove the restriction that netdev must be up before a wdev can be
>> obtained in nl80211, or changing channels in cfg80211.
>>
>> Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
>>
>> ---
>> Hi list,
>>
>> This was encountered while implementing an interface for setting
>> BSSBasicRateSet in mesh. The wdev->channel is needed in nl80211.c for
>> rate verification, but prior to this patch not available.  This doesn't
>> seem right since the following sequence of commands would fail:
>>
>> iw wlan0 set type mp
>> iw wlan0 set channel n
>> ip link set wlan0 up
>> iw wlan0 mesh join meshid basic-rate 1,2
>>
>> Also, 'iw set channel' is met with an -EBUSY if doing this after the
>> link is up for fixed channel modes (mesh) anyway.
>>
>> Comments? Any idea why this was required initially?
>
>
> I don't think this patch is correct -- mac80211 will get updated even if
> the device isn't even started which will likely cause trouble. Even if
> not though, this all doesn't match any kind of multi-channel concept. We
> treat the channel as part of the temporary setup, e.g. part of the
> association. AP and mesh are the only ones that are different today I
> think.
>
> Overall, I don't think setting the channel & doing mesh setup separately
> is really a good API.
>
> From mac80211's (and other drivers if they existed) POV the channel
> should be given with the mesh_join command, like for IBSS.
>
> Now, obviously, requiring userspace to do that would be an API change.
> We probably don't want that, so I would suggest to change cfg80211 to
> track the channel and then pass it to join_mesh as one of the mesh
> parameters. This could be made work even when somebody attempts to set
> the channel before the interface is up, but we'd have to be careful
> about interface type changes.

Unfortunately, when __nl80211_set_channel() is called we may or may
not have a wdev, so there is nowhere to store the interface-specific
channel and type.
We could shove these into a cfg80211_registered_device, but now just
"last channel for this wiphy" is tracked, and that doesn't seem to
help your multi-channel operation.

If the above is correct, how about we leave the existing API as-is and
simply extend join_mesh to take a channel attribute?

Also, with IBSS the desired channel is pushed to the driver along with
the setup parameters. What do you think about calling
__nl80211_set_channel() directly instead of relying on the cfg80211
driver to handle this?

> We should do the same for AP mode as well, since the channel really
> becomes relevant only upon start_ap(), before that there's no real
> concept of a channel since you don't use it yet anyway.

Thanks,
Thomas

Re: [RFC] nl80211: don't require netdev UP for wdev

[Johannes Berg]

Hi Thomas,


> > I don't think this patch is correct -- mac80211 will get updated even if
> > the device isn't even started which will likely cause trouble. Even if
> > not though, this all doesn't match any kind of multi-channel concept. We
> > treat the channel as part of the temporary setup, e.g. part of the
> > association. AP and mesh are the only ones that are different today I
> > think.
> >
> > Overall, I don't think setting the channel & doing mesh setup separately
> > is really a good API.
> >
> > From mac80211's (and other drivers if they existed) POV the channel
> > should be given with the mesh_join command, like for IBSS.
> >
> > Now, obviously, requiring userspace to do that would be an API change.
> > We probably don't want that, so I would suggest to change cfg80211 to
> > track the channel and then pass it to join_mesh as one of the mesh
> > parameters. This could be made work even when somebody attempts to set
> > the channel before the interface is up, but we'd have to be careful
> > about interface type changes.
> 
> Unfortunately, when __nl80211_set_channel() is called we may or may
> not have a wdev, so there is nowhere to store the interface-specific
> channel and type.

Well, but if you don't have a wdev, is the setting relevant at all? Or
does it take effect later? This all isn't very well-defined I guess.

> We could shove these into a cfg80211_registered_device, but now just
> "last channel for this wiphy" is tracked, and that doesn't seem to
> help your multi-channel operation.

So you'd want to allow setting the channel on the phy, and have it take
effect for mesh? Is that really something we need to support? I'd think
almost everyone would set the channel on the netdev?

> If the above is correct, how about we leave the existing API as-is and
> simply extend join_mesh to take a channel attribute?

We could, but that'd mean that it can be NULL if the user doesn't set
it, which seems a bit odd to me too and then the driver again would have
to sort it out. I'd prefer if we could sort it out in cfg80211 so the
driver (mac80211) is simpler.

> Also, with IBSS the desired channel is pushed to the driver along with
> the setup parameters. What do you think about calling
> __nl80211_set_channel() directly instead of relying on the cfg80211
> driver to handle this?

No, that's certainly not possible. In IBSS the channel is just the
default channel if we don't find an IBSS. And in any case I'd rather
call set_channel less than more.

johannes

Re: [RFC] nl80211: don't require netdev UP for wdev

[Thomas Pedersen]

On Thu, May 10, 2012 at 11:12 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Hi Thomas,
>
>
>> > I don't think this patch is correct -- mac80211 will get updated even if
>> > the device isn't even started which will likely cause trouble. Even if
>> > not though, this all doesn't match any kind of multi-channel concept. We
>> > treat the channel as part of the temporary setup, e.g. part of the
>> > association. AP and mesh are the only ones that are different today I
>> > think.
>> >
>> > Overall, I don't think setting the channel & doing mesh setup separately
>> > is really a good API.
>> >
>> > From mac80211's (and other drivers if they existed) POV the channel
>> > should be given with the mesh_join command, like for IBSS.
>> >
>> > Now, obviously, requiring userspace to do that would be an API change.
>> > We probably don't want that, so I would suggest to change cfg80211 to
>> > track the channel and then pass it to join_mesh as one of the mesh
>> > parameters. This could be made work even when somebody attempts to set
>> > the channel before the interface is up, but we'd have to be careful
>> > about interface type changes.
>>
>> Unfortunately, when __nl80211_set_channel() is called we may or may
>> not have a wdev, so there is nowhere to store the interface-specific
>> channel and type.
>
> Well, but if you don't have a wdev, is the setting relevant at all? Or
> does it take effect later? This all isn't very well-defined I guess.
>
>> We could shove these into a cfg80211_registered_device, but now just
>> "last channel for this wiphy" is tracked, and that doesn't seem to
>> help your multi-channel operation.
>
> So you'd want to allow setting the channel on the phy, and have it take
> effect for mesh? Is that really something we need to support? I'd think
> almost everyone would set the channel on the netdev?

No :) Thanks for clearing that up.

>> If the above is correct, how about we leave the existing API as-is and
>> simply extend join_mesh to take a channel attribute?
>
> We could, but that'd mean that it can be NULL if the user doesn't set
> it, which seems a bit odd to me too and then the driver again would have
> to sort it out. I'd prefer if we could sort it out in cfg80211 so the
> driver (mac80211) is simpler.

For this, we can store a default channel and type in the default mesh config.

>> Also, with IBSS the desired channel is pushed to the driver along with
>> the setup parameters. What do you think about calling
>> __nl80211_set_channel() directly instead of relying on the cfg80211
>> driver to handle this?
>
> No, that's certainly not possible. In IBSS the channel is just the
> default channel if we don't find an IBSS. And in any case I'd rather
> call set_channel less than more.

So cfg80211_set_freq() from cfg80211_join_mesh() is out, too?

Re: [RFC] nl80211: don't require netdev UP for wdev

[Johannes Berg]

On Thu, 2012-05-10 at 12:47 -0700, Thomas Pedersen wrote:

> > We could, but that'd mean that it can be NULL if the user doesn't set
> > it, which seems a bit odd to me too and then the driver again would have
> > to sort it out. I'd prefer if we could sort it out in cfg80211 so the
> > driver (mac80211) is simpler.
> 
> For this, we can store a default channel and type in the default mesh config.

That would still break older userspace though, it would get the default
channel instead of the channel it set before, right?

> >> Also, with IBSS the desired channel is pushed to the driver along with
> >> the setup parameters. What do you think about calling
> >> __nl80211_set_channel() directly instead of relying on the cfg80211
> >> driver to handle this?
> >
> > No, that's certainly not possible. In IBSS the channel is just the
> > default channel if we don't find an IBSS. And in any case I'd rather
> > call set_channel less than more.
> 
> So cfg80211_set_freq() from cfg80211_join_mesh() is out, too?

I think we should just pass the channel to the join_mesh callback.

I just did this a bit for AP mode. Now, in AP mode we have one
advantage: we can rely on userspace setting the channel because
hostapd/wpa_s always does that. For mesh, we may not have that luxury?

For AP, it looks roughly like this:
http://p.sipsolutions.net/35913f571ea5bd43.txt

Maybe we can do something similar for mesh and mostly get rid of setting
the channel directly.

johannes