Since commit bc19d6e0b74ef03a3baf035412c95192b54dfc6f entitled
"b43/b43legacy: add RFKILL_STATE_HARD_BLOCKED support", the radio LED
no longer responds to rfkill switch events.
ATM, I do not have a fix, other than reversion of this commit.
Larry
The LED state was not being updated by rfkill_force_state(), which will
cause regressions in wireless drivers that had old-style rfkill support and
are updated to use rfkill_force_state().
The LED state was not being updated when a change was detected through the
rfkill->get_state() hook, either.
Move the LED trigger update calls into notify_rfkill_state_change(), where
it should have been in the first place. This takes care of both issues.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
net/rfkill/rfkill.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
John, this one is quite likely something that should be sent for
merge in mainline BEFORE 2.6.27 is released.
I am NOT sure it fixes regressions, that depends on whether the drivers
using rfkill that are in 2.6.27 had working LED support before rfkill
support was added to them. Unfortunately, it cannot fix the b43
regression by itself.
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index ea0dc04..f949a48 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -125,6 +125,7 @@ static void rfkill_led_trigger_activate(struct led_classdev *led)
static void notify_rfkill_state_change(struct rfkill *rfkill)
{
+ rfkill_led_trigger(rfkill, rfkill->state);
blocking_notifier_call_chain(&rfkill_notifier_list,
RFKILL_STATE_CHANGED,
rfkill);
@@ -217,10 +218,8 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
rfkill->state = state;
}
- if (force || rfkill->state != oldstate) {
- rfkill_led_trigger(rfkill, rfkill->state);
+ if (force || rfkill->state != oldstate)
notify_rfkill_state_change(rfkill);
- }
return retval;
}
--
1.5.6.5
On Wed, 17 Sep 2008, Larry Finger wrote:
> Henrique de Moraes Holschuh wrote:
> > He means rfkill_force_state() should change the LED state in the process,
> > which it isn't doing.
> >
> > It is an one-line patch, I am testing it now.
>
> If your one-line patch fixes my regression, then I'll ACK it for 2.6.27.
Sorry, it can't fix that regression by itself, since b43 is not using
rfkill_force_state() in mainline.
It will fix the same issue you are experiencing on b43 in anything that uses
rfkill_force_state(), which means (for mainline): iwlwifi, rt2x00,
thinkpad-acpi and hp-wmi.
It is a regression only on b43 and thinkpad-acpi, as the other drivers above
didn't use rfkill_force_state() in 2.6.26.
To fix the regression in b43, you'd be faced with the need of both my simple
patch AND Matthew's more complex one.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wednesday 17 September 2008 00:37:29 Henrique de Moraes Holschuh wrote:
> On Tue, 16 Sep 2008, Michael Buesch wrote:
> > But I don't know how to tell the rfkill subsystem about the states and
>
> rfkill_force_state(). Must NOT be called from within atomic contextes,
> something I haven't got around to find a proper way of fixing, and nobody
> else seems to be on a rfkill coding frenzy right now.
That's a showstopper for us, as we have to change the state from
within an interrupt tasklet.
> I won't go on the rfkill-allocate/-free stuff, messing with that API means
> you need to fix a lot of other people's drivers. But you have the
> rfkill-hw-state-changed now, it is called rfkill_force_state(). The only
> crap is that it cannot be called from atomic contexts.
Yeah well. I didn't know. I lost interest in rfkill pretty much when it
stopped blowing up (until it started again when somebody merged a patch
recently).
--
Greetings Michael.
On Wed, 17 Sep 2008, Michael Buesch wrote:
> On Wednesday 17 September 2008 01:32:40 Matthew Garrett wrote:
> > On Tue, Sep 16, 2008 at 02:30:35PM -0500, Larry Finger wrote:
> >
> > > I didn't say it was not possible. What I said is that _ONLY_ the
> > > operator's finger could change the state, just like in your laptop.
> > > Thus it makes absolutely no difference what state RFKILL thinks it is
> > > in.
> >
> > Of course it makes a difference. The reason why two states are provided
> > is to allow userspace to distinguish whether it can unblock the device
> > or not. It's clear that b43's rfkill code is astonishingly broken (and
> > that's not a criticism of anyone involved - the documentation's
> > confusing and there weren't any good examples of how it should be
> > implemented).
> >
> > The real question is how the LED state is supposed to be being toggled,
> > and what that's got to do with rfkill. I /think/ that the current state
> > of events is:
>
> Read the rfkill code. It toggles a LED trigger if the state changes from
> UNBLOCKED to BLOCKED. b43 uses that trigger to run the radio LED.
>
> > 1) User toggles state
> > 2) b43 changes rfkill state (by using rfkill_force_state). The LED state
> > should also be changed in the process.
>
> No it shouldn't. LEDs are entirely handled by triggers. We must _never_ toggle
> the LED state from within b43 directly via hardcoded stuff.
> rfkill is responsible for handling the radio LEDs in the machine.
He means rfkill_force_state() should change the LED state in the process,
which it isn't doing.
It is an one-line patch, I am testing it now.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wednesday 17 September 2008 17:18:22 Henrique de Moraes Holschuh wrote:
> On Wed, 17 Sep 2008, Michael Buesch wrote:
> > On Tuesday 16 September 2008 23:09:20 Matthew Garrett wrote:
> > > Oh, hey, I suck. This one might stand a better chance of not falling
> > > over.
> > >
> > > diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
> > > index fec5645..991e318 100644
> > > --- a/drivers/net/wireless/b43/rfkill.c
> > > +++ b/drivers/net/wireless/b43/rfkill.c
> > > @@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
> > > struct b43_rfkill *rfk = &(dev->wl->rfkill);
> > >
> > > if (!dev->radio_hw_enable) {
> > > - rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
> > > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
> > > return;
> > > }
> > >
> > > if (!dev->phy.radio_on)
> > > - rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
> > > else
> > > - rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
> > > -
> > > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
> > > }
> >
> > I still thing that it is really wrong to check and change the software
> > rfkill state from within the _hardware_ rfkill state handler.
> > So let's say this handler sets the state to SOFT_BLOCKED. Who is going
> > to set it back to unblocked, if the user unblocks the radio?
>
> I would suggest reading the updated docs, but I know you'd rather just stay
> away from rfkill.
>
> For reference:
>
> Basically, your wireless device driver has to monitor any hardware rfkill
> lines and call rfkill_force_state() accordingly (you don't even need to
> track if the state changed, rfkill core will do that).
>
> The call to rfkill_force_state() should set state to HARD_BLOCKED (any of
> the hardware rfkill lines are active), SOFT_BLOCKED (no hardware rfkill
> lines are active, but one of the software rfkill lines are active), or
> UNBLOCKED (no rfkill lines of any type are active).
>
> Whether this is to be done through interrupts or pooling is something that
> depends on your driver and the hardware.
>
> User and userspace interaction is taken care of by the rfkill core. You do
> nothing of the sort in the wireless device driver.
>
> There are *very few* exceptions for the above as far as wireless device
> drivers go. They are explained in the docs, and I know of no wireless
> device driver that would require any them.
>
> > We can turn the radio on/off from the mac80211 config callback. Possibly
> > we must tell rfkill about it (I'm not sure. I don't understand the API).
> > I think this is pretty hard to get right, actually. HW-block and SW-block
> > are two completely independent states in b43. You can HW-block and SW-block
> > the device at the same time. So one must make sure that at any time the rfkill
> > is in a sane state if _either_ HW-block or SW-block changes. Currently I think
> > we only change rfkill state if HW-block state changes. Which is wrong.
>
> Again, for reference:
>
> You need to synthesize a single state (unblock/soft-block/hard-block) for a
> transmitter from every rfkill line that affects that transmitter. This
> happens because the interface is a SINGLE ONE rfkill class per independent
> wireless interface ("transmitter").
Right. But that's not what I was talking about. The hw and sw rfkill is something
_completely_ different in b43 and it's handled by _completely_ different codepaths.
I just said that currently the sw-rfkill path does _not_ announce the state
correctly to the rfkill subsystem. Additionally we must _not_ check the sw rfkill
state from within the hw rfkill handlers, as it will get out of sync this way.
(that's what we're currently doing. If you revert the commit from the subject
this will go away, afaics).
Telling rfkill about the state isn't the hard part, but the
"You need to synthesize a single state" part, which currently is not done correctly.
We currently have _two_ states. (Don't get me wrong, the fix is _not_ to remove
either of these. The fix is to introduce a common rfkill notifier that constructs
a common rfkill state from these two states. This notifier is called from hw-rfkill
and sw-rfkill.)
--
Greetings Michael.
On Wed, 17 Sep 2008, Matthew Garrett wrote:
> that's not a criticism of anyone involved - the documentation's
> confusing and there weren't any good examples of how it should be
> implemented).
I sure hope you mean "the documentation WAS confusing"... because if it is
still confusing, I have not got any comments or ideas about how it could be
improved yet... (HINT!).
> The real question is how the LED state is supposed to be being toggled,
> and what that's got to do with rfkill. I /think/ that the current state
> of events is:
>
> 1) User toggles state
> 2) State toggle is used by b43 to generate KEY_WLAN (this is broken)
> 3) rfkill-input toggles the rfkill state, changing the LED state in the
> process
Actually 2 and 3 will fight each other, and weird things will happen.
> What *should* be happening is:
>
> 1) User toggles state
> 2) b43 changes rfkill state (by using rfkill_force_state). The LED state
> should also be changed in the process.
Correct.
However, rfkill_force_state() is NOT updating LED states (I just checked).
I will sleep on the issue, and send in a patch tomorrow.
This probably means a small patch to rfkill + Matthew's fixed patch to use
rfkill_force_state() in b43 will fix the regression in the right way.
I don't care either way which kind of fix goes to 2.6.27, though.
The proper fix for rfkill will be in two stages. A small fix now, and a
complete change on the LED handling to use the blocking notifier chain
instead later on (which will clean up rfkill code somewhat).
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wednesday 17 September 2008 16:29:31 John W. Linville wrote:
> On Wed, Sep 17, 2008 at 04:26:28PM +0200, Michael Buesch wrote:
> > On Wednesday 17 September 2008 00:37:29 Henrique de Moraes Holschuh wrote:
> > > On Tue, 16 Sep 2008, Michael Buesch wrote:
> > > > But I don't know how to tell the rfkill subsystem about the states and
> > >
> > > rfkill_force_state(). Must NOT be called from within atomic contextes,
> > > something I haven't got around to find a proper way of fixing, and nobody
> > > else seems to be on a rfkill coding frenzy right now.
> >
> > That's a showstopper for us, as we have to change the state from
> > within an interrupt tasklet.
>
> Workqueue?
Yeah, that's possible to implement yet another workaround using a workqueue.
However it would be nice to have rfkill either support atomic context
or implement the workaround in the rfkill core, as I'm sure there are more
devices that report rfkill changes via interrupt.
--
Greetings Michael.
Henrique de Moraes Holschuh wrote:
>
> However, rfkill_force_state() is NOT updating LED states (I just checked).
> I will sleep on the issue, and send in a patch tomorrow.
>
> This probably means a small patch to rfkill + Matthew's fixed patch to use
> rfkill_force_state() in b43 will fix the regression in the right way.
>
> I don't care either way which kind of fix goes to 2.6.27, though.
>
> The proper fix for rfkill will be in two stages. A small fix now, and a
> complete change on the LED handling to use the blocking notifier chain
> instead later on (which will clean up rfkill code somewhat).
>
I do not dispute that rfkill-handling in b43 is broken; however, prior
to the commit in question, it worked.
I also think we can agree that we need to get it working before 2.6.27
is released. If the small fix now is the reversion of bc19d6e, then I
think this is the correct path. We will then have a couple of weeks to
get the code working correctly before the 2.6.28 merge starts.
I admit that I never tested any of the RFKILL patches as they went in.
One of the reasons is that the development process seemed rather
untidy to an outsider, and I wasn't sure that any of the code would
ever be in the kernel. As such, it snuck up on me. I'll not let that
happen again. After the reversion, I will again test any suggested
code changes, but do not expect me to work out any of the changes. I
have enough to do.
Larry
On Tuesday 16 September 2008 21:18:27 Carlos Corbacho wrote:
> On Tuesday 16 September 2008 18:08:48 Larry Finger wrote:
> > I agree with Michael. From what I know, the only possible reason for
> > having [RFKILL_STATE_HARD_BLOCKED] would be if user space could
> > somehow affect the state of the hardware switch.
>
> When I disable the b43 device in my laptop via acer-wmi (which in turn, calls
> into the laptops firmware), b43 physically cannot re-enable it (a not
> uncommon case on a lot of laptops). In which case, as far as b43 is
> concerned, the wireless radio is then in RFKILL_STATE_HARD_BLOCKED, since b43
> is unable to re-enable the radio on the hardware.
>
> So yes, it is quite possible for b43 to be in RFKILL_STATE_HARD_BLOCKED.
Sure it is. It's the common case, if there's a button.
But I don't know how to tell the rfkill subsystem about the states and
I do not _want_ to. See, that crap code _always_ blew up for every single
patch we did. I'm tired of this and I am not interested in that anymore.
Please revert that patch and if that makes it work I'm fine with it.
I'm still waiting for the sane rfkill API where we have three functions
rfkill-allocate
rfkill-hw-state-changed
rfkill-free
That's all I need. All that input stuff, which was _the_ way to go some
months ago, but is now deprecated it seems (but I don't know what it it
replaced by) is just so really confusing.
--
Greetings Michael.
On Wed, 17 Sep 2008, Larry Finger wrote:
> Henrique de Moraes Holschuh wrote:
> > The LED state was not being updated by rfkill_force_state(), which will
> > cause regressions in wireless drivers that had old-style rfkill support and
> > are updated to use rfkill_force_state().
> >
> > The LED state was not being updated when a change was detected through the
> > rfkill->get_state() hook, either.
> >
> > Move the LED trigger update calls into notify_rfkill_state_change(), where
> > it should have been in the first place. This takes care of both issues.
> >
> > Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> > Cc: Ivo van Doorn <[email protected]>
> > ---
> > net/rfkill/rfkill.c | 5 ++---
> > 1 files changed, 2 insertions(+), 3 deletions(-)
> >
> > John, this one is quite likely something that should be sent for
> > merge in mainline BEFORE 2.6.27 is released.
> >
> > I am NOT sure it fixes regressions, that depends on whether the drivers
> > using rfkill that are in 2.6.27 had working LED support before rfkill
> > support was added to them. Unfortunately, it cannot fix the b43
> > regression by itself.
>
> The b43 regression is not fixed with this patch and the one from
> Matthew that starts out with "Oh, hey, I suck. This one might stand a
> better chance of not falling over."
Curious. My patch to rfkill WAS tested, and it DOES fix the same issue you
reported (hardware state changes to HARD_BLOCKED do not update the LEDs) in
thinkpad-acpi. It is also an "obviously correct" patch.
What this probably means is that b43 would need a little more rfkill surgery
than what Matthew's patch already did. I will look over Matthew's patch,
but my guess is that Michael's comments about the need to add some extra
code to b43 to actually synthesize the rfkill state from the separate HW and
SW rfkill input lines are a strong hint of where the problem might be.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
> diff --git a/drivers/net/wireless/b43/rfkill.c
> b/drivers/net/wireless/b43/rfkill.c
> index fec5645..e8b2acb 100644
> --- a/drivers/net/wireless/b43/rfkill.c
> +++ b/drivers/net/wireless/b43/rfkill.c
> @@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev
> *dev)
> struct b43_rfkill *rfk = &(dev->wl->rfkill);
>
> if (!dev->radio_hw_enable) {
> - rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
> return;
> }
>
> if (!dev->phy.radio_on)
> - rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
> else
> - rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
> -
> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
> }
Just this should be enough to fix the regression for 2.6.27 (and should be
trivial to port to b43legacy) in the immediate term (I was planning to
send something similar).
The rest of the patch is out-of-scope for 2.6.27, but is definitely along
he right lines, IMHO (and it would be good if we can get a working version
of it in to 2.6.28). I may try and look at it later, as I do have my b43
hardware handy.
-Carlos
--
E-Mail: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D
Henrique de Moraes Holschuh wrote:
>
> Curious. My patch to rfkill WAS tested, and it DOES fix the same issue you
> reported (hardware state changes to HARD_BLOCKED do not update the LEDs) in
> thinkpad-acpi. It is also an "obviously correct" patch.
>
> What this probably means is that b43 would need a little more rfkill surgery
> than what Matthew's patch already did. I will look over Matthew's patch,
> but my guess is that Michael's comments about the need to add some extra
> code to b43 to actually synthesize the rfkill state from the separate HW and
> SW rfkill input lines are a strong hint of where the problem might be.
You must have missed my mail that shows the corrected patch for b43.
See http://marc.info/?l=linux-wireless&m=122171448920267&w=2.
Larry
On Wed, Sep 17, 2008 at 6:59 PM, Michael Buesch <[email protected]> wrote:
> On Wednesday 17 September 2008 17:18:22 Henrique de Moraes Holschuh wrote:
>> On Wed, 17 Sep 2008, Michael Buesch wrote:
>> > On Tuesday 16 September 2008 23:09:20 Matthew Garrett wrote:
>> > > Oh, hey, I suck. This one might stand a better chance of not falling
>> > > over.
>> > >
>> > > diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
>> > > index fec5645..991e318 100644
>> > > --- a/drivers/net/wireless/b43/rfkill.c
>> > > +++ b/drivers/net/wireless/b43/rfkill.c
>> > > @@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
>> > > struct b43_rfkill *rfk = &(dev->wl->rfkill);
>> > >
>> > > if (!dev->radio_hw_enable) {
>> > > - rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
>> > > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
>> > > return;
>> > > }
>> > >
>> > > if (!dev->phy.radio_on)
>> > > - rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
>> > > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
>> > > else
>> > > - rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
>> > > -
>> > > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
>> > > }
>> >
>> > I still thing that it is really wrong to check and change the software
>> > rfkill state from within the _hardware_ rfkill state handler.
>> > So let's say this handler sets the state to SOFT_BLOCKED. Who is going
>> > to set it back to unblocked, if the user unblocks the radio?
>>
>> I would suggest reading the updated docs, but I know you'd rather just stay
>> away from rfkill.
>>
>> For reference:
>>
>> Basically, your wireless device driver has to monitor any hardware rfkill
>> lines and call rfkill_force_state() accordingly (you don't even need to
>> track if the state changed, rfkill core will do that).
>>
>> The call to rfkill_force_state() should set state to HARD_BLOCKED (any of
>> the hardware rfkill lines are active), SOFT_BLOCKED (no hardware rfkill
>> lines are active, but one of the software rfkill lines are active), or
>> UNBLOCKED (no rfkill lines of any type are active).
>>
>> Whether this is to be done through interrupts or pooling is something that
>> depends on your driver and the hardware.
>>
>> User and userspace interaction is taken care of by the rfkill core. You do
>> nothing of the sort in the wireless device driver.
>>
>> There are *very few* exceptions for the above as far as wireless device
>> drivers go. They are explained in the docs, and I know of no wireless
>> device driver that would require any them.
>>
>> > We can turn the radio on/off from the mac80211 config callback. Possibly
>> > we must tell rfkill about it (I'm not sure. I don't understand the API).
>> > I think this is pretty hard to get right, actually. HW-block and SW-block
>> > are two completely independent states in b43. You can HW-block and SW-block
>> > the device at the same time. So one must make sure that at any time the rfkill
>> > is in a sane state if _either_ HW-block or SW-block changes. Currently I think
>> > we only change rfkill state if HW-block state changes. Which is wrong.
>>
>> Again, for reference:
>>
>> You need to synthesize a single state (unblock/soft-block/hard-block) for a
>> transmitter from every rfkill line that affects that transmitter. This
>> happens because the interface is a SINGLE ONE rfkill class per independent
>> wireless interface ("transmitter").
>
> Right. But that's not what I was talking about. The hw and sw rfkill is something
> _completely_ different in b43 and it's handled by _completely_ different codepaths.
> I just said that currently the sw-rfkill path does _not_ announce the state
> correctly to the rfkill subsystem. Additionally we must _not_ check the sw rfkill
> state from within the hw rfkill handlers, as it will get out of sync this way.
> (that's what we're currently doing. If you revert the commit from the subject
> this will go away, afaics).
> Telling rfkill about the state isn't the hard part, but the
> "You need to synthesize a single state" part, which currently is not done correctly.
> We currently have _two_ states. (Don't get me wrong, the fix is _not_ to remove
> either of these. The fix is to introduce a common rfkill notifier that constructs
> a common rfkill state from these two states. This notifier is called from hw-rfkill
> and sw-rfkill.)
In practice the rfkill in WiFi comes to 2 common cases
HW|SW
Most of the world SW and HW are independent states and radio is
enabled only in 0|0
0|1
/ \
0|0 - - 1|1
\ /
1|0
Dell Only - Unblocking HW rfkill enabled radio.
0|1
/ \
0|0 -- <-HW- - 1|1
\ /
1|0
This is why all vendors (including iwlwifi) that targets notebook and
friends markets need to implement independent SW HW rfkill switches
and we need 2 bits (4 states- 3 states are not enought I've wrote
that couple of times....
Thanks
Tomas
On Wed, 17 Sep 2008, Michael Buesch wrote:
> On Tuesday 16 September 2008 23:09:20 Matthew Garrett wrote:
> > Oh, hey, I suck. This one might stand a better chance of not falling
> > over.
> >
> > diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
> > index fec5645..991e318 100644
> > --- a/drivers/net/wireless/b43/rfkill.c
> > +++ b/drivers/net/wireless/b43/rfkill.c
> > @@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
> > struct b43_rfkill *rfk = &(dev->wl->rfkill);
> >
> > if (!dev->radio_hw_enable) {
> > - rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
> > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
> > return;
> > }
> >
> > if (!dev->phy.radio_on)
> > - rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
> > else
> > - rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
> > -
> > + rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
> > }
>
> I still thing that it is really wrong to check and change the software
> rfkill state from within the _hardware_ rfkill state handler.
> So let's say this handler sets the state to SOFT_BLOCKED. Who is going
> to set it back to unblocked, if the user unblocks the radio?
I would suggest reading the updated docs, but I know you'd rather just stay
away from rfkill.
For reference:
Basically, your wireless device driver has to monitor any hardware rfkill
lines and call rfkill_force_state() accordingly (you don't even need to
track if the state changed, rfkill core will do that).
The call to rfkill_force_state() should set state to HARD_BLOCKED (any of
the hardware rfkill lines are active), SOFT_BLOCKED (no hardware rfkill
lines are active, but one of the software rfkill lines are active), or
UNBLOCKED (no rfkill lines of any type are active).
Whether this is to be done through interrupts or pooling is something that
depends on your driver and the hardware.
User and userspace interaction is taken care of by the rfkill core. You do
nothing of the sort in the wireless device driver.
There are *very few* exceptions for the above as far as wireless device
drivers go. They are explained in the docs, and I know of no wireless
device driver that would require any them.
> We can turn the radio on/off from the mac80211 config callback. Possibly
> we must tell rfkill about it (I'm not sure. I don't understand the API).
> I think this is pretty hard to get right, actually. HW-block and SW-block
> are two completely independent states in b43. You can HW-block and SW-block
> the device at the same time. So one must make sure that at any time the rfkill
> is in a sane state if _either_ HW-block or SW-block changes. Currently I think
> we only change rfkill state if HW-block state changes. Which is wrong.
Again, for reference:
You need to synthesize a single state (unblock/soft-block/hard-block) for a
transmitter from every rfkill line that affects that transmitter. This
happens because the interface is a SINGLE ONE rfkill class per independent
wireless interface ("transmitter").
> In the end, I do not care about this crap anymore.
That's quite understandable... b43 is one of the drivers (if not THE
driver) that suffered most from rfkill bugs, as it was an early adopter and
the old rfkill API was just plain broken for the kind of stuff b43 needed it
for.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, 16 Sep 2008, Michael Buesch wrote:
> But I don't know how to tell the rfkill subsystem about the states and
rfkill_force_state(). Must NOT be called from within atomic contextes,
something I haven't got around to find a proper way of fixing, and nobody
else seems to be on a rfkill coding frenzy right now.
> I'm still waiting for the sane rfkill API where we have three functions
> rfkill-allocate
> rfkill-hw-state-changed
> rfkill-free
I won't go on the rfkill-allocate/-free stuff, messing with that API means
you need to fix a lot of other people's drivers. But you have the
rfkill-hw-state-changed now, it is called rfkill_force_state(). The only
crap is that it cannot be called from atomic contexts.
> months ago, but is now deprecated it seems (but I don't know what it it
> replaced by) is just so really confusing.
Read Documentation/rfkill.txt. And the kernel-doc comments on rfkill.c.
The rfkill documentation was updated.
If you still have doubts after reading the stuff above, ask on
linux-wireless and CC me.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, Sep 16, 2008 at 12:08:48PM -0500, Larry Finger wrote:
> I agree with Michael. From what I know, the only possible reason for
> having this state would be if user space could somehow affect the
> state of the hardware switch. As the user's finger is the only such
> thing, then there is no use for the RFKILL_STATE_HARD_BLOCKED state,
> particularly when it breaks LED operations.
RFKILL_STATE_HARD_BLOCKED indicates that the hardware is disabled in a
way that userspace can't influence, so sounds like exactly the right
state to have here. I still have absolutely no idea what b43 rfkill is
supposed to be doing - why on earth is it requesting rfkill-input, and
why does it generate keypress events? I /think/ it should e something
like the following (untested, I'm not near any of my b43 hardware at the
moment). This basically does the following:
1) Split the update function in two, so it can be called by either
polling or an interrupt driven event on newer hardware (not implemented
yet)
2) Remove all the input handling
3) Change the state updates to use rfkill_force_state, which will
generate an event that gets sent up to userland
4) Retains the RFKILL_STATE_HARD_BLOCKED code
When the user flicks a switch or presses a button that physically
disables the radio, the state will now automatically change to
RFKILL_STATE_HARD_BLOCKED. If they have a key that generates KEY_WLAN
but doesn't change the radio state, rfkill-input will trigger a change
to RFKILL_STATE_SOFT_BLOCKED.
Like I said, this is only build-tested - I won't be back at a b43 until
next week. If someone could give this a go, that would be great.
diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
index fec5645..e8b2acb 100644
--- a/drivers/net/wireless/b43/rfkill.c
+++ b/drivers/net/wireless/b43/rfkill.c
@@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
struct b43_rfkill *rfk = &(dev->wl->rfkill);
if (!dev->radio_hw_enable) {
- rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
+ rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
return;
}
if (!dev->phy.radio_on)
- rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
+ rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
else
- rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
-
+ rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
}
-/* The poll callback for the hardware button. */
-static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
+static void b43_rfkill_update(struct b43_wldev *dev)
{
- struct b43_wldev *dev = poll_dev->private;
struct b43_wl *wl = dev->wl;
bool enabled;
bool report_change = 0;
@@ -82,13 +79,25 @@ static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
enabled ? "ENABLED" : "DISABLED");
}
mutex_unlock(&wl->mutex);
+}
- /* send the radio switch event to the system - note both a key press
- * and a release are required */
- if (unlikely(report_change)) {
- input_report_key(poll_dev->input, KEY_WLAN, 1);
- input_report_key(poll_dev->input, KEY_WLAN, 0);
- }
+static void b43_rfkill_poll(unsigned long data)
+{
+ struct b43_rfkill *rfkill = (struct b43_rfkill *)data;
+ schedule_work(&rfkill->poll_queue);
+}
+
+static void b43_rfkill_work(struct work_struct *work)
+{
+ struct b43_rfkill *rfk = container_of(work, struct b43_rfkill,
+ poll_queue);
+ struct b43_wl *wl = container_of(rfk, struct b43_wl, rfkill);
+ struct b43_wldev *dev = wl->current_dev;
+
+ b43_rfkill_update(dev);
+ rfk->poll_timer.function = b43_rfkill_poll;
+ rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
+ add_timer(&rfk->poll_timer);
}
/* Called when the RFKILL toggled in software. */
@@ -158,48 +167,23 @@ void b43_rfkill_init(struct b43_wldev *dev)
rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
rfk->rfkill->user_claim_unsupported = 1;
- rfk->poll_dev = input_allocate_polled_device();
- if (!rfk->poll_dev) {
- rfkill_free(rfk->rfkill);
- goto err_freed_rfk;
- }
-
- rfk->poll_dev->private = dev;
- rfk->poll_dev->poll = b43_rfkill_poll;
- rfk->poll_dev->poll_interval = 1000; /* msecs */
-
- rfk->poll_dev->input->name = rfk->name;
- rfk->poll_dev->input->id.bustype = BUS_HOST;
- rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
- rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
- set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
-
err = rfkill_register(rfk->rfkill);
if (err)
- goto err_free_polldev;
+ goto err_free_rfk;
-#ifdef CONFIG_RFKILL_INPUT_MODULE
- /* B43 RF-kill isn't useful without the rfkill-input subsystem.
- * Try to load the module. */
- err = request_module("rfkill-input");
- if (err)
- b43warn(wl, "Failed to load the rfkill-input module. "
- "The built-in radio LED will not work.\n");
-#endif /* CONFIG_RFKILL_INPUT */
+ rfk->registered = 1;
- err = input_register_polled_device(rfk->poll_dev);
- if (err)
- goto err_unreg_rfk;
+ INIT_WORK(&rfk->poll_queue, b43_rfkill_work);
- rfk->registered = 1;
+ init_timer(&rfk->poll_timer);
+ rfk->poll_timer.function = b43_rfkill_poll;
+ rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
+ rfk->poll_timer.data = (unsigned long)dev;
+ add_timer(&rfk->poll_timer);
return;
-err_unreg_rfk:
- rfkill_unregister(rfk->rfkill);
-err_free_polldev:
- input_free_polled_device(rfk->poll_dev);
- rfk->poll_dev = NULL;
-err_freed_rfk:
+err_free_rfk:
+ rfkill_free(rfk->rfkill);
rfk->rfkill = NULL;
out_error:
rfk->registered = 0;
@@ -214,9 +198,8 @@ void b43_rfkill_exit(struct b43_wldev *dev)
return;
rfk->registered = 0;
- input_unregister_polled_device(rfk->poll_dev);
+ del_timer(&rfk->poll_timer);
+
rfkill_unregister(rfk->rfkill);
- input_free_polled_device(rfk->poll_dev);
- rfk->poll_dev = NULL;
rfk->rfkill = NULL;
}
diff --git a/drivers/net/wireless/b43/rfkill.h b/drivers/net/wireless/b43/rfkill.h
index adacf93..1a4a719 100644
--- a/drivers/net/wireless/b43/rfkill.h
+++ b/drivers/net/wireless/b43/rfkill.h
@@ -7,14 +7,14 @@ struct b43_wldev;
#ifdef CONFIG_B43_RFKILL
#include <linux/rfkill.h>
-#include <linux/input-polldev.h>
-
struct b43_rfkill {
/* The RFKILL subsystem data structure */
struct rfkill *rfkill;
- /* The poll device for the RFKILL input button */
- struct input_polled_dev *poll_dev;
+ /* The timer list for the RFKILL polling */
+ struct timer_list poll_timer;
+ /* Workqueue for the RFKILL polling */
+ struct work_struct poll_queue;
/* Did initialization succeed? Used for freeing. */
bool registered;
/* The unique name of this rfkill switch */
--
Matthew Garrett | [email protected]
On Wednesday 17 September 2008, Henrique de Moraes Holschuh wrote:
> The LED state was not being updated by rfkill_force_state(), which will
> cause regressions in wireless drivers that had old-style rfkill support and
> are updated to use rfkill_force_state().
>
> The LED state was not being updated when a change was detected through the
> rfkill->get_state() hook, either.
>
> Move the LED trigger update calls into notify_rfkill_state_change(), where
> it should have been in the first place. This takes care of both issues.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> John, this one is quite likely something that should be sent for
> merge in mainline BEFORE 2.6.27 is released.
>
> I am NOT sure it fixes regressions, that depends on whether the drivers
> using rfkill that are in 2.6.27 had working LED support before rfkill
> support was added to them. Unfortunately, it cannot fix the b43
> regression by itself.
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index ea0dc04..f949a48 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -125,6 +125,7 @@ static void rfkill_led_trigger_activate(struct led_classdev *led)
>
> static void notify_rfkill_state_change(struct rfkill *rfkill)
> {
> + rfkill_led_trigger(rfkill, rfkill->state);
> blocking_notifier_call_chain(&rfkill_notifier_list,
> RFKILL_STATE_CHANGED,
> rfkill);
> @@ -217,10 +218,8 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
> rfkill->state = state;
> }
>
> - if (force || rfkill->state != oldstate) {
> - rfkill_led_trigger(rfkill, rfkill->state);
> + if (force || rfkill->state != oldstate)
> notify_rfkill_state_change(rfkill);
> - }
>
> return retval;
> }
Carlos Corbacho wrote:
> On Tuesday 16 September 2008 18:08:48 Larry Finger wrote:
>> I agree with Michael. From what I know, the only possible reason for
>> having [RFKILL_STATE_HARD_BLOCKED] would be if user space could
>> somehow affect the state of the hardware switch.
>
> When I disable the b43 device in my laptop via acer-wmi (which in turn, calls
> into the laptops firmware), b43 physically cannot re-enable it (a not
> uncommon case on a lot of laptops). In which case, as far as b43 is
> concerned, the wireless radio is then in RFKILL_STATE_HARD_BLOCKED, since b43
> is unable to re-enable the radio on the hardware.
>
> So yes, it is quite possible for b43 to be in RFKILL_STATE_HARD_BLOCKED.
I didn't say it was not possible. What I said is that _ONLY_ the
operator's finger could change the state, just like in your laptop.
Thus it makes absolutely no difference what state RFKILL thinks it is
in. The driver knows what state the software wants, and can read but
not write the hardware state. There has to be a bug in RFKILL that got
exposed when this commit was included. This regression needs to be
fixed ASAP, otherwise 2.6.27 will be released with it. If the RFKILL
guys want to fix their bug, I'll be happy to test code, but not now.
Larry
On Wed, 17 Sep 2008, Tomas Winkler wrote:
> In practice the rfkill in WiFi comes to 2 common cases
> HW|SW
>
> Most of the world SW and HW are independent states and radio is
> enabled only in 0|0
> 0|1
> / \
> 0|0 - - 1|1
> \ /
> 1|0
Indeed. And it is like that, because that's how WiFi hardware with hardware
rfkill input lines work in the bare metal.
> Dell Only - Unblocking HW rfkill enabled radio.
> 0|1
> / \
> 0|0 -- <-HW- - 1|1
> \ /
> 1|0
This is likely Dell's crap of a firmware getting in the way.
Still, does the Dell firmware interferes just when the HW rfkill input line
changes state (i.e. on edges), or all the time?
> This is why all vendors (including iwlwifi) that targets notebook and
> friends markets need to implement independent SW HW rfkill switches
> and we need 2 bits (4 states- 3 states are not enought I've wrote
> that couple of times....
You need two bits inside the drivers, yes.
However, the core interface (rfkill) does not. It has a different point of
view, one that is driven by the needs of the interface and userspace
applications, and not by hardware implementation.
The rfkill core has three states:
0 - Radio blocked, but could be unblocked by software request
1 - Radio unblocked, can always be blocked by software request
2 - Radio blocked and locked, software request cannot unblock it
These states are defined by the valid [for rfkill] combinations of two
SEMANTHIC parameters: "the current state can be changed by the
rfkill->toggle_radio() callback" and "transmitter is currently emmiting
energy".
There is no fourth state, because a radio locked into unblocked mode is
completely unacceptable for rfkill: it is against all safety regulations for
the kind of wireless transmitter rfkill is concerned with.
The reason why the underlying wireless driver can or cannot change the
transmitter state through rfkill->toggle_radio() is NOT important to the
rfkill core. And if it were, IMHO we would be better off by exporting that
as a separate attribute, and not messing with the rfkill states.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tuesday 16 September 2008 16:18:34 Larry Finger wrote:
> Since commit bc19d6e0b74ef03a3baf035412c95192b54dfc6f entitled
> "b43/b43legacy: add RFKILL_STATE_HARD_BLOCKED support", the radio LED
> no longer responds to rfkill switch events.
>
> ATM, I do not have a fix, other than reversion of this commit.
It's kind of bogus that it pokes with the software rfkill state
from within the hardware rfkill handler.
Please revert this patch. The commit log says nothing about
why this is needed and I don't see a point in overriding the state
in the rfkill-private data structure from within the driver.
--
Greetings Michael.
Oh, hey, I suck. This one might stand a better chance of not falling
over.
diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
index fec5645..991e318 100644
--- a/drivers/net/wireless/b43/rfkill.c
+++ b/drivers/net/wireless/b43/rfkill.c
@@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
struct b43_rfkill *rfk = &(dev->wl->rfkill);
if (!dev->radio_hw_enable) {
- rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
+ rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
return;
}
if (!dev->phy.radio_on)
- rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
+ rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
else
- rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
-
+ rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
}
-/* The poll callback for the hardware button. */
-static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
+static void b43_rfkill_update(struct b43_wldev *dev)
{
- struct b43_wldev *dev = poll_dev->private;
struct b43_wl *wl = dev->wl;
bool enabled;
bool report_change = 0;
@@ -82,13 +79,25 @@ static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
enabled ? "ENABLED" : "DISABLED");
}
mutex_unlock(&wl->mutex);
+}
- /* send the radio switch event to the system - note both a key press
- * and a release are required */
- if (unlikely(report_change)) {
- input_report_key(poll_dev->input, KEY_WLAN, 1);
- input_report_key(poll_dev->input, KEY_WLAN, 0);
- }
+static void b43_rfkill_poll(unsigned long data)
+{
+ struct b43_rfkill *rfkill = (struct b43_rfkill *)data;
+ schedule_work(&rfkill->poll_queue);
+}
+
+static void b43_rfkill_work(struct work_struct *work)
+{
+ struct b43_rfkill *rfk = container_of(work, struct b43_rfkill,
+ poll_queue);
+ struct b43_wl *wl = container_of(rfk, struct b43_wl, rfkill);
+ struct b43_wldev *dev = wl->current_dev;
+
+ b43_rfkill_update(dev);
+ rfk->poll_timer.function = b43_rfkill_poll;
+ rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
+ add_timer(&rfk->poll_timer);
}
/* Called when the RFKILL toggled in software. */
@@ -158,48 +167,23 @@ void b43_rfkill_init(struct b43_wldev *dev)
rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
rfk->rfkill->user_claim_unsupported = 1;
- rfk->poll_dev = input_allocate_polled_device();
- if (!rfk->poll_dev) {
- rfkill_free(rfk->rfkill);
- goto err_freed_rfk;
- }
-
- rfk->poll_dev->private = dev;
- rfk->poll_dev->poll = b43_rfkill_poll;
- rfk->poll_dev->poll_interval = 1000; /* msecs */
-
- rfk->poll_dev->input->name = rfk->name;
- rfk->poll_dev->input->id.bustype = BUS_HOST;
- rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
- rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
- set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
-
err = rfkill_register(rfk->rfkill);
if (err)
- goto err_free_polldev;
+ goto err_free_rfk;
-#ifdef CONFIG_RFKILL_INPUT_MODULE
- /* B43 RF-kill isn't useful without the rfkill-input subsystem.
- * Try to load the module. */
- err = request_module("rfkill-input");
- if (err)
- b43warn(wl, "Failed to load the rfkill-input module. "
- "The built-in radio LED will not work.\n");
-#endif /* CONFIG_RFKILL_INPUT */
+ rfk->registered = 1;
- err = input_register_polled_device(rfk->poll_dev);
- if (err)
- goto err_unreg_rfk;
+ INIT_WORK(&rfk->poll_queue, b43_rfkill_work);
- rfk->registered = 1;
+ init_timer(&rfk->poll_timer);
+ rfk->poll_timer.function = b43_rfkill_poll;
+ rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
+ rfk->poll_timer.data = (unsigned long)rfk;
+ add_timer(&rfk->poll_timer);
return;
-err_unreg_rfk:
- rfkill_unregister(rfk->rfkill);
-err_free_polldev:
- input_free_polled_device(rfk->poll_dev);
- rfk->poll_dev = NULL;
-err_freed_rfk:
+err_free_rfk:
+ rfkill_free(rfk->rfkill);
rfk->rfkill = NULL;
out_error:
rfk->registered = 0;
@@ -214,9 +198,8 @@ void b43_rfkill_exit(struct b43_wldev *dev)
return;
rfk->registered = 0;
- input_unregister_polled_device(rfk->poll_dev);
+ del_timer(&rfk->poll_timer);
+
rfkill_unregister(rfk->rfkill);
- input_free_polled_device(rfk->poll_dev);
- rfk->poll_dev = NULL;
rfk->rfkill = NULL;
}
diff --git a/drivers/net/wireless/b43/rfkill.h b/drivers/net/wireless/b43/rfkill.h
index adacf93..1a4a719 100644
--- a/drivers/net/wireless/b43/rfkill.h
+++ b/drivers/net/wireless/b43/rfkill.h
@@ -7,14 +7,14 @@ struct b43_wldev;
#ifdef CONFIG_B43_RFKILL
#include <linux/rfkill.h>
-#include <linux/input-polldev.h>
-
struct b43_rfkill {
/* The RFKILL subsystem data structure */
struct rfkill *rfkill;
- /* The poll device for the RFKILL input button */
- struct input_polled_dev *poll_dev;
+ /* The timer list for the RFKILL polling */
+ struct timer_list poll_timer;
+ /* Workqueue for the RFKILL polling */
+ struct work_struct poll_queue;
/* Did initialization succeed? Used for freeing. */
bool registered;
/* The unique name of this rfkill switch */
--
Matthew Garrett | [email protected]
On Tue, Sep 16, 2008 at 09:52:12PM -0500, Larry Finger wrote:
> I admit that I never tested any of the RFKILL patches as they went in.
> One of the reasons is that the development process seemed rather
> untidy to an outsider, and I wasn't sure that any of the code would
> ever be in the kernel. As such, it snuck up on me. I'll not let that
> happen again. After the reversion, I will again test any suggested
> code changes, but do not expect me to work out any of the changes. I
> have enough to do.
FWIW, Henrique has been very persistant with driving rfkill. He has
posted and reposted his patch series many times. I'm sorry that it
snuck-up on you but it was on the list for quite a while.
John
--
John W. Linville
[email protected]
Matthew Garrett wrote:
> On Tue, Sep 16, 2008 at 12:08:48PM -0500, Larry Finger wrote:
>
>> I agree with Michael. From what I know, the only possible reason for
>> having this state would be if user space could somehow affect the
>> state of the hardware switch. As the user's finger is the only such
>> thing, then there is no use for the RFKILL_STATE_HARD_BLOCKED state,
>> particularly when it breaks LED operations.
>
> RFKILL_STATE_HARD_BLOCKED indicates that the hardware is disabled in a
> way that userspace can't influence, so sounds like exactly the right
> state to have here. I still have absolutely no idea what b43 rfkill is
> supposed to be doing - why on earth is it requesting rfkill-input, and
> why does it generate keypress events? I /think/ it should e something
> like the following (untested, I'm not near any of my b43 hardware at the
> moment). This basically does the following:
>
> 1) Split the update function in two, so it can be called by either
> polling or an interrupt driven event on newer hardware (not implemented
> yet)
> 2) Remove all the input handling
> 3) Change the state updates to use rfkill_force_state, which will
> generate an event that gets sent up to userland
> 4) Retains the RFKILL_STATE_HARD_BLOCKED code
>
> When the user flicks a switch or presses a button that physically
> disables the radio, the state will now automatically change to
> RFKILL_STATE_HARD_BLOCKED. If they have a key that generates KEY_WLAN
> but doesn't change the radio state, rfkill-input will trigger a change
> to RFKILL_STATE_SOFT_BLOCKED.
>
> Like I said, this is only build-tested - I won't be back at a b43 until
> next week. If someone could give this a go, that would be great.
It locks up tight with a kernel panic when booting. I have a screen
picture that I will send separately to Matthew.
Larry
Michael Buesch wrote:
> On Tuesday 16 September 2008 16:18:34 Larry Finger wrote:
>> Since commit bc19d6e0b74ef03a3baf035412c95192b54dfc6f entitled
>> "b43/b43legacy: add RFKILL_STATE_HARD_BLOCKED support", the radio LED
>> no longer responds to rfkill switch events.
>>
>> ATM, I do not have a fix, other than reversion of this commit.
>
> It's kind of bogus that it pokes with the software rfkill state
> from within the hardware rfkill handler.
>
> Please revert this patch. The commit log says nothing about
> why this is needed and I don't see a point in overriding the state
> in the rfkill-private data structure from within the driver.
I agree with Michael. From what I know, the only possible reason for
having this state would be if user space could somehow affect the
state of the hardware switch. As the user's finger is the only such
thing, then there is no use for the RFKILL_STATE_HARD_BLOCKED state,
particularly when it breaks LED operations.
Please revert the patch.
Larry
On Tuesday 16 September 2008 18:08:48 Larry Finger wrote:
> I agree with Michael. From what I know, the only possible reason for
> having [RFKILL_STATE_HARD_BLOCKED] would be if user space could
> somehow affect the state of the hardware switch.
When I disable the b43 device in my laptop via acer-wmi (which in turn, calls
into the laptops firmware), b43 physically cannot re-enable it (a not
uncommon case on a lot of laptops). In which case, as far as b43 is
concerned, the wireless radio is then in RFKILL_STATE_HARD_BLOCKED, since b43
is unable to re-enable the radio on the hardware.
So yes, it is quite possible for b43 to be in RFKILL_STATE_HARD_BLOCKED.
-Carlos
--
E-Mail: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D
On Wednesday 17 September 2008 01:32:40 Matthew Garrett wrote:
> On Tue, Sep 16, 2008 at 02:30:35PM -0500, Larry Finger wrote:
>
> > I didn't say it was not possible. What I said is that _ONLY_ the
> > operator's finger could change the state, just like in your laptop.
> > Thus it makes absolutely no difference what state RFKILL thinks it is
> > in.
>
> Of course it makes a difference. The reason why two states are provided
> is to allow userspace to distinguish whether it can unblock the device
> or not. It's clear that b43's rfkill code is astonishingly broken (and
> that's not a criticism of anyone involved - the documentation's
> confusing and there weren't any good examples of how it should be
> implemented).
>
> The real question is how the LED state is supposed to be being toggled,
> and what that's got to do with rfkill. I /think/ that the current state
> of events is:
Read the rfkill code. It toggles a LED trigger if the state changes from
UNBLOCKED to BLOCKED. b43 uses that trigger to run the radio LED.
> 1) User toggles state
> 2) b43 changes rfkill state (by using rfkill_force_state). The LED state
> should also be changed in the process.
No it shouldn't. LEDs are entirely handled by triggers. We must _never_ toggle
the LED state from within b43 directly via hardcoded stuff.
rfkill is responsible for handling the radio LEDs in the machine.
--
Greetings Michael.
Henrique de Moraes Holschuh wrote:
> The LED state was not being updated by rfkill_force_state(), which will
> cause regressions in wireless drivers that had old-style rfkill support and
> are updated to use rfkill_force_state().
>
> The LED state was not being updated when a change was detected through the
> rfkill->get_state() hook, either.
>
> Move the LED trigger update calls into notify_rfkill_state_change(), where
> it should have been in the first place. This takes care of both issues.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> John, this one is quite likely something that should be sent for
> merge in mainline BEFORE 2.6.27 is released.
>
> I am NOT sure it fixes regressions, that depends on whether the drivers
> using rfkill that are in 2.6.27 had working LED support before rfkill
> support was added to them. Unfortunately, it cannot fix the b43
> regression by itself.
The b43 regression is not fixed with this patch and the one from
Matthew that starts out with "Oh, hey, I suck. This one might stand a
better chance of not falling over."
Larry
On Tuesday 16 September 2008 23:09:20 Matthew Garrett wrote:
> Oh, hey, I suck. This one might stand a better chance of not falling
> over.
>
> diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
> index fec5645..991e318 100644
> --- a/drivers/net/wireless/b43/rfkill.c
> +++ b/drivers/net/wireless/b43/rfkill.c
> @@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
> struct b43_rfkill *rfk = &(dev->wl->rfkill);
>
> if (!dev->radio_hw_enable) {
> - rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
> return;
> }
>
> if (!dev->phy.radio_on)
> - rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
> else
> - rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
> -
> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
> }
I still thing that it is really wrong to check and change the software
rfkill state from within the _hardware_ rfkill state handler.
So let's say this handler sets the state to SOFT_BLOCKED. Who is going
to set it back to unblocked, if the user unblocks the radio?
We can turn the radio on/off from the mac80211 config callback. Possibly
we must tell rfkill about it (I'm not sure. I don't understand the API).
I think this is pretty hard to get right, actually. HW-block and SW-block
are two completely independent states in b43. You can HW-block and SW-block
the device at the same time. So one must make sure that at any time the rfkill
is in a sane state if _either_ HW-block or SW-block changes. Currently I think
we only change rfkill state if HW-block state changes. Which is wrong.
In the end, I do not care about this crap anymore.
Feel free to remove my copyright notice from that file and put yours in
there. So feel free to send any patch to the rfkill code to john, but please
handle any regressions resulting from it. If not, I will handle them by reverting
patches until it starts to work again. ;)
--
Greetings Michael.
On Thu, 18 Sep 2008, Larry Finger wrote:
> Henrique de Moraes Holschuh wrote:
> > Curious. My patch to rfkill WAS tested, and it DOES fix the same issue you
> > reported (hardware state changes to HARD_BLOCKED do not update the LEDs) in
> > thinkpad-acpi. It is also an "obviously correct" patch.
> >
> > What this probably means is that b43 would need a little more rfkill surgery
> > than what Matthew's patch already did. I will look over Matthew's patch,
> > but my guess is that Michael's comments about the need to add some extra
> > code to b43 to actually synthesize the rfkill state from the separate HW and
> > SW rfkill input lines are a strong hint of where the problem might be.
>
> You must have missed my mail that shows the corrected patch for b43.
> See http://marc.info/?l=linux-wireless&m=122171448920267&w=2.
Indeed I have. I will read it now and comment on it.
Thanks for the heads' up :-)
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, Sep 16, 2008 at 02:30:35PM -0500, Larry Finger wrote:
> I didn't say it was not possible. What I said is that _ONLY_ the
> operator's finger could change the state, just like in your laptop.
> Thus it makes absolutely no difference what state RFKILL thinks it is
> in.
Of course it makes a difference. The reason why two states are provided
is to allow userspace to distinguish whether it can unblock the device
or not. It's clear that b43's rfkill code is astonishingly broken (and
that's not a criticism of anyone involved - the documentation's
confusing and there weren't any good examples of how it should be
implemented).
The real question is how the LED state is supposed to be being toggled,
and what that's got to do with rfkill. I /think/ that the current state
of events is:
1) User toggles state
2) State toggle is used by b43 to generate KEY_WLAN (this is broken)
3) rfkill-input toggles the rfkill state, changing the LED state in the
process
What *should* be happening is:
1) User toggles state
2) b43 changes rfkill state (by using rfkill_force_state). The LED state
should also be changed in the process.
It looks like the commit in question gets part of the way to
implementing the second of these situations, but will now change the
rfkill state at the same time that rfkill-input tries to. I agree that
it should be reverted for now, but because it's (a) broken (changing the
state member directly is wrong) and (b) potentially open to strange
states being generated by rfkill-input trying to change the state after
it's already been changed. The general intent is correct.
--
Matthew Garrett | [email protected]
Henrique de Moraes Holschuh wrote:
>
> He means rfkill_force_state() should change the LED state in the process,
> which it isn't doing.
>
> It is an one-line patch, I am testing it now.
>
If your one-line patch fixes my regression, then I'll ACK it for 2.6.27.
Larry
On Tue, 16 Sep 2008, Larry Finger wrote:
> This one doesn't crash, but it doesn't turn the light off.
Hmm... if this lack of LED control when HARD_BLOCKED is a problem in rfkill,
I will track it down and fix it.
The rest of the stuff (b43's usage of rfkill), is a problem for the people
who know the b43 driver to fix, I don't dare touch it and break it further.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Carlos Corbacho wrote:
>> diff --git a/drivers/net/wireless/b43/rfkill.c
>> b/drivers/net/wireless/b43/rfkill.c
>> index fec5645..e8b2acb 100644
>> --- a/drivers/net/wireless/b43/rfkill.c
>> +++ b/drivers/net/wireless/b43/rfkill.c
>> @@ -49,21 +49,18 @@ static void b43_rfkill_update_state(struct b43_wldev
>> *dev)
>> struct b43_rfkill *rfk = &(dev->wl->rfkill);
>>
>> if (!dev->radio_hw_enable) {
>> - rfk->rfkill->state = RFKILL_STATE_HARD_BLOCKED;
>> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_HARD_BLOCKED);
>> return;
>> }
>>
>> if (!dev->phy.radio_on)
>> - rfk->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
>> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_SOFT_BLOCKED);
>> else
>> - rfk->rfkill->state = RFKILL_STATE_UNBLOCKED;
>> -
>> + rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
>> }
>
> Just this should be enough to fix the regression for 2.6.27 (and should be
> trivial to port to b43legacy) in the immediate term (I was planning to
> send something similar).
>
> The rest of the patch is out-of-scope for 2.6.27, but is definitely along
> he right lines, IMHO (and it would be good if we can get a working version
> of it in to 2.6.28). I may try and look at it later, as I do have my b43
> hardware handy.
This one doesn't crash, but it doesn't turn the light off.
I'm done testing for now!
John,
Revert the patch until the rfkill guys get their house in order.
Larry
On Wed, Sep 17, 2008 at 04:26:28PM +0200, Michael Buesch wrote:
> On Wednesday 17 September 2008 00:37:29 Henrique de Moraes Holschuh wrote:
> > On Tue, 16 Sep 2008, Michael Buesch wrote:
> > > But I don't know how to tell the rfkill subsystem about the states and
> >
> > rfkill_force_state(). Must NOT be called from within atomic contextes,
> > something I haven't got around to find a proper way of fixing, and nobody
> > else seems to be on a rfkill coding frenzy right now.
>
> That's a showstopper for us, as we have to change the state from
> within an interrupt tasklet.
Workqueue?
--
John W. Linville
[email protected]
Henrique de Moraes Holschuh wrote:
>
> Sorry, it can't fix that regression by itself, since b43 is not using
> rfkill_force_state() in mainline.
>
> It will fix the same issue you are experiencing on b43 in anything that uses
> rfkill_force_state(), which means (for mainline): iwlwifi, rt2x00,
> thinkpad-acpi and hp-wmi.
>
> It is a regression only on b43 and thinkpad-acpi, as the other drivers above
> didn't use rfkill_force_state() in 2.6.26.
>
> To fix the regression in b43, you'd be faced with the need of both my simple
> patch AND Matthew's more complex one.
I don't expect that the more complex one will be accepted into 2.6.27.
Larry