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?
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
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
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.
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
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
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
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
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 <[email protected]>
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;
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
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