Re: Dscape ieee80211: enabling/disabling the radio

[Ivo van Doorn <ivdoorn@gmail.com>]

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

On Wednesday 10 May 2006 12:42, Jiri Benc wrote:
> On Sat, 6 May 2006 14:00:58 +0200, Ivo van Doorn wrote:
> > In rt2x00 the config() handler schedules all configuration changes by using a workqueue,
> > this is required since several configuration changes in rt2x00 need sleeping and for
> > USB devices all register access requires sleeping. And the config() handler is often
> > called from interrupt context so it complains a lot when the workqueue is not used.
> > 
> > This seemed fine, untill the radio_enabled field was introduced to the configuration structure.
> > When the radio_enable field is set, the radio must be enabled, but enabling
> > the radio is something that can (at least in rt2x00) fail. So scheduling the enabling of the radio
> > to the workqueue is not something that is desired since the stack can not be notified that the
> > device is not able to enable the radio.
> 
> This is probably more general problem. I can imagine a card that will
> need to sleep to switch channels and can fail. This doesn't matter now
> (as we cannot handle failure in switching channels) but it should be
> solved in some way.
> 
> Currently, only switching channels is made in interrupt context. You can
> depend on this for now.
> 
> > Moving the enabling of the radio outside the workqueue function and into the config()
> > handler results in scheduling while atomic issues since the enabling of the radio requires
> > sleeping for both PCI and USB devices.
> 
> Or am I wrong? I see no place when radio_enabled is changed in interrupt context.

I think the problem is what the dscape exactly expects the driver to do when
add_interface() is called by the stack. When that call has finished, does the stack
expects the radio to be enabled, or should it instruct the driver to enable the radio later
on through a field in the config structure.

What currently happens in rt2x00 (Which might not be doing what the stack expects)
is that when add_interface() is called the radio remains off untill open() is called or
radio_enabled has been set in the config structure.
What happens is that the first call of config() (which is being made before open()) comes
from the scan handler requesting a channel change, which is in interrupt context, but
when the channel is changed, the radio is also supposed to be enabled which results
in the problems of radio enabling requirement to be scheduled.

But this might be resolved by demanding that the driver is keeping track of the number
of interfaces added, and enabling the radio when at least 1 interface has been added
and only disable the radio when all interfaces have been removed. And completely
remove the open() and stop() handlers since they would only confuse matters.

> > Instead of using a config field radio_enabled, wouldn't it be better to add 2 handlers
> > to the ieee80211_hw structure, something like enable_radio() and disable_radio()?
> > If these functions are called from normal context the dscape stack can still enable
> > and disable the radio whenever it is desired, and it is able to check the return value
> > to see if the request has actually succeeded.
> 
> I don't think this is the best idea. In such case we would need to move
> almost all the stuff from ieee80211_conf to separate functions. This is
> something we don't want.

True, but I actually meant that these 2 functions would replace the open() and stop() functions.
Which is not required if the above suggestion of the number of added interfaces counter
is applied to the drivers.

> > What I am wondering about afterwards is what exactly should happen when the open()
> > and stop() handlers are being called? Because those are basicly intented to enable
> > and stop the radio as well. I checked bcm43xx to see what they do, and they don't seem
> > to check the radio_enabled field, so I don't know what they do besides enabling the radio.
> 
> open and stop callbacks are obsolete. They were replaced by
> add_interface and remove_interface callbacks and will be removed after
> drivers are converted to use the new callbacks.

Ah ok, I'll fix rt2x00 then to do the correct behaviour and no longer rely on open() and stop()
for the radio, and add the counter for the number of interfaces that are up so the radio
will behave in the correct fashion. :)

Thanks.

Ivo

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