zd1211 config operation problem

zd1211 config operation problem

[Michael Buesch]

The zd1211 driver has a bug in the ieee80211 config callback.
It currently unconditionally calls the set_channel() function,
whether the device is running or not. This causes a failed
bus access, if we set some config while the device is down.
The driver must check if the device is running before setting
any config on it (usb->initialized?).
I tried to fix this, but I failed to find the correct condition
to check for.

I'm also wondering why we only set the channel in the config
callback. Is the rest all unsupported by the device, like
setting of TX power and so on?

Re: zd1211 config operation problem

[Ulrich Kunitz]

Michael Buesch wrote:

> The zd1211 driver has a bug in the ieee80211 config callback.
> It currently unconditionally calls the set_channel() function,
> whether the device is running or not. This causes a failed
> bus access, if we set some config while the device is down.
> The driver must check if the device is running before setting
> any config on it (usb->initialized?).
> I tried to fix this, but I failed to find the correct condition
> to check for.

Thank you for highlighting this. My MAC address patch introduced a
flag mac->open, which could be useful here. We would need to
store the channel and set it, when the device is opened. One could
ask why the MAC stack is not handling this.

> I'm also wondering why we only set the channel in the config
> callback. Is the rest all unsupported by the device, like
> setting of TX power and so on?

We don't know how to do it; we are missing good documentation
about the hardware registers. The different RFs will also be a
factor.

-- 
Uli Kunitz

Re: zd1211 config operation problem

[Michael Wu]

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

On Sunday 23 September 2007 10:48, Michael Buesch wrote:
> The zd1211 driver has a bug in the ieee80211 config callback.
> It currently unconditionally calls the set_channel() function,
> whether the device is running or not. This causes a failed
> bus access, if we set some config while the device is down.
> The driver must check if the device is running before setting
> any config on it (usb->initialized?).
Hm, that sounds pretty terrible. mac80211 should check if the device was 
started before trying to run any configure callbacks.

-Michael Wu

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

Re: zd1211 config operation problem

[Michael Buesch]

On Sunday 23 September 2007, Michael Wu wrote:
> On Sunday 23 September 2007 10:48, Michael Buesch wrote:
> > The zd1211 driver has a bug in the ieee80211 config callback.
> > It currently unconditionally calls the set_channel() function,
> > whether the device is running or not. This causes a failed
> > bus access, if we set some config while the device is down.
> > The driver must check if the device is running before setting
> > any config on it (usb->initialized?).
> Hm, that sounds pretty terrible. mac80211 should check if the device was 
> started before trying to run any configure callbacks.

Something like that?




Subject: mac80211: Check open_count before calling config callback.

Also remove the check for ops->config!=NULL, as it can never be NULL.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

Index: wireless-dev/net/mac80211/ieee80211.c
===================================================================
--- wireless-dev.orig/net/mac80211/ieee80211.c	2007-09-24 18:29:54.000000000 +0200
+++ wireless-dev/net/mac80211/ieee80211.c	2007-09-24 18:36:11.000000000 +0200
@@ -722,7 +722,7 @@ int ieee80211_hw_config(struct ieee80211
 	       local->hw.conf.phymode);
 #endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
 
-	if (local->ops->config)
+	if (local->open_count)
 		ret = local->ops->config(local_to_hw(local), &local->hw.conf);
 
 	return ret;

Re: zd1211 config operation problem

[Johannes Berg]

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

On Mon, 2007-09-24 at 18:41 +0200, Michael Buesch wrote:

> -	if (local->ops->config)
> +	if (local->open_count)
>  		ret = local->ops->config(local_to_hw(local), &local->hw.conf);

Isn't this racy?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: zd1211 config operation problem

[Johannes Berg]

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

On Mon, 2007-09-24 at 19:20 +0200, Johannes Berg wrote:
> On Mon, 2007-09-24 at 18:41 +0200, Michael Buesch wrote:
> 
> > -	if (local->ops->config)
> > +	if (local->open_count)
> >  		ret = local->ops->config(local_to_hw(local), &local->hw.conf);
> 
> Isn't this racy?

Also, shouldn't we then call ->config when the first interface is
brought up or something?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: zd1211 config operation problem

[Michael Wu]

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

On Monday 24 September 2007 13:20, Johannes Berg wrote:
> On Mon, 2007-09-24 at 18:41 +0200, Michael Buesch wrote:
> > -	if (local->ops->config)
> > +	if (local->open_count)
> >  		ret = local->ops->config(local_to_hw(local), &local->hw.conf);
> Isn't this racy?
>
Shouldn't be. Some callers are holding rtnl (ieee80211_open, 
ieee80211_ioctl*). Other callers (in ieee80211_sta.c) cannot run if the 
device is down.

-Michael Wu

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

Re: zd1211 config operation problem

[Michael Wu]

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

On Monday 24 September 2007 13:21, Johannes Berg wrote:
> Also, shouldn't we then call ->config when the first interface is
> brought up or something?
>
I'm pretty sure that was done for all cases at some point. When did it get 
switched to being done for just monitor?

-Michael Wu

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

Re: zd1211 config operation problem

[Johannes Berg]

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

On Mon, 2007-09-24 at 15:20 -0400, Michael Wu wrote:
> On Monday 24 September 2007 13:20, Johannes Berg wrote:
> > On Mon, 2007-09-24 at 18:41 +0200, Michael Buesch wrote:
> > > -	if (local->ops->config)
> > > +	if (local->open_count)
> > >  		ret = local->ops->config(local_to_hw(local), &local->hw.conf);
> > Isn't this racy?
> >
> Shouldn't be. Some callers are holding rtnl (ieee80211_open, 
> ieee80211_ioctl*). Other callers (in ieee80211_sta.c) cannot run if the 
> device is down.

Good point. I guess the patch is fine then.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: zd1211 config operation problem

[Johannes Berg]

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

On Mon, 2007-09-24 at 15:22 -0400, Michael Wu wrote:

> I'm pretty sure that was done for all cases at some point. When did it get 
> switched to being done for just monitor?

Looks like an oversight in the filter flags patch. I'll take a look
tomorrow.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: zd1211 config operation problem

[Michael Buesch]

On Monday 24 September 2007, Johannes Berg wrote:
> On Mon, 2007-09-24 at 19:20 +0200, Johannes Berg wrote:
> > On Mon, 2007-09-24 at 18:41 +0200, Michael Buesch wrote:
> > 
> > > -	if (local->ops->config)
> > > +	if (local->open_count)
> > >  		ret = local->ops->config(local_to_hw(local), &local->hw.conf);
> > 
> > Isn't this racy?
> 
> Also, shouldn't we then call ->config when the first interface is
> brought up or something?

I don't know. You are the one who knows the code. ;)
I'm just pointing at problems that I'm not able to fix,
because I don't know the code.