[PATCH] rfkill: always init poll delayed work

[PATCH] rfkill: always init poll delayed work

[Johannes Berg]

The rfkill core didn't initialise the poll delayed work
because it assumed that polling was always done by specifying
the poll function. cfg80211, however, would like to start
polling only later, which is a valid use case and easy to
support, so change rfkill to always initialise the poll
delayed work and thus allow starting polling by calling the
rfkill_resume_polling() function after registration.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/rfkill/core.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

--- wireless-testing.orig/net/rfkill/core.c	2009-06-03 09:52:39.000000000 +0200
+++ wireless-testing/net/rfkill/core.c	2009-06-03 09:53:03.000000000 +0200
@@ -909,16 +909,15 @@ int __must_check rfkill_register(struct 
 
 	rfkill->registered = true;
 
-	if (rfkill->ops->poll) {
-		INIT_DELAYED_WORK(&rfkill->poll_work, rfkill_poll);
-		schedule_delayed_work(&rfkill->poll_work,
-			round_jiffies_relative(POLL_INTERVAL));
-	}
-
+	INIT_DELAYED_WORK(&rfkill->poll_work, rfkill_poll);
 	INIT_WORK(&rfkill->uevent_work, rfkill_uevent_work);
-
 	INIT_WORK(&rfkill->sync_work, rfkill_sync_work);
+
+	if (rfkill->ops->poll)
+		schedule_delayed_work(&rfkill->poll_work,
+			round_jiffies_relative(POLL_INTERVAL));
 	schedule_work(&rfkill->sync_work);
+
 	rfkill_send_events(rfkill, RFKILL_OP_ADD);
 
 	mutex_unlock(&rfkill_global_mutex);

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:
> The rfkill core didn't initialise the poll delayed work
> because it assumed that polling was always done by specifying
> the poll function. cfg80211, however, would like to start
> polling only later, which is a valid use case and easy to
> support, so change rfkill to always initialise the poll
> delayed work and thus allow starting polling by calling the
> rfkill_resume_polling() function after registration.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---

This patch fixed to kernel BUG_ON, but there are still problems. This
has been tested with b43.

1. A minor one - the radio LED switch never comes on. Module led-class
is loaded.

2. Much more serious - When the radio kill switch is turned off, the
radio is killed just as expected, but it is not restored when the
switch is turned on. The only way to restore the radio is to
rmmod/insmod b43. Similarly, if the module is loaded with the switch
off, it is not possible to turn the radio on. An unload/load resquence
is then needed.

Larry

Re: [PATCH] rfkill: always init poll delayed work

[Johannes Berg]

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

On Wed, 2009-06-03 at 09:50 -0500, Larry Finger wrote:

> This patch fixed to kernel BUG_ON, but there are still problems. This
> has been tested with b43.

Thanks.

> 1. A minor one - the radio LED switch never comes on. Module led-class
> is loaded.

Hmm, that's odd, but I linked it to mac80211's radio LED trigger now, I
think. Can you verify that?

> 2. Much more serious - When the radio kill switch is turned off, the
> radio is killed just as expected, but it is not restored when the
> switch is turned on. The only way to restore the radio is to
> rmmod/insmod b43. Similarly, if the module is loaded with the switch
> off, it is not possible to turn the radio on. An unload/load resquence
> is then needed.

I suspected that much. And you can't recover that since you can't set
the interface UP. This is because polling doesn't work while the
interface is set down. As I said previously, I think that's previously
been buggy too, if you did
 1) hard kill
 2) set interface down
 3) hard unkill

then step 3) would not trigger an event to userspace until you set the
interface up again, afaict.

We probably need to bring up the core to poll it, if possible?

johannes

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

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:
> On Wed, 2009-06-03 at 09:50 -0500, Larry Finger wrote:
>> 2. Much more serious - When the radio kill switch is turned off, the
>> radio is killed just as expected, but it is not restored when the
>> switch is turned on. The only way to restore the radio is to
>> rmmod/insmod b43. Similarly, if the module is loaded with the switch
>> off, it is not possible to turn the radio on. An unload/load resquence
>> is then needed.
> 
> I suspected that much. And you can't recover that since you can't set
> the interface UP. This is because polling doesn't work while the
> interface is set down. As I said previously, I think that's previously
> been buggy too, if you did
>  1) hard kill
>  2) set interface down
>  3) hard unkill
> 
> then step 3) would not trigger an event to userspace until you set the
> interface up again, afaict.
> 
> We probably need to bring up the core to poll it, if possible?

I have not made much progress in sorting this out. When I turn the
switch off, I see RFKILL_STATE in /sys/class/rfkill/rfkill0/uevent go
from unblocked to hw_blocked. It does not change when the switch is
turned on.

I have verified that b43_rfkill_poll(), the polling callback routine
is being executed, but that the hardware bit in the interface is never
being set again. Whichever part of the old rfkill_input code that made
that change seems not to be functioning.

What diagnostics would be helpful?

Larry

Re: [PATCH] rfkill: always init poll delayed work

[Johannes Berg]

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

On Thu, 2009-06-04 at 09:52 -0500, Larry Finger wrote:

> I have not made much progress in sorting this out. When I turn the
> switch off, I see RFKILL_STATE in /sys/class/rfkill/rfkill0/uevent go
> from unblocked to hw_blocked. It does not change when the switch is
> turned on.
> 
> I have verified that b43_rfkill_poll(), the polling callback routine
> is being executed, but that the hardware bit in the interface is never
> being set again. Whichever part of the old rfkill_input code that made
> that change seems not to be functioning.
> 
> What diagnostics would be helpful?

Could you try, without the patch in question, what happens in terms of
uevents
 * have everything on
 * press rfkill button (expect -> hw_blocked transition)
 * put interface down
 * press rfkill button (expect -> unblocked transition)
 * put interface up

please?

johannes

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

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:
> 
> Could you try, without the patch in question, what happens in terms of
> uevents
>  * have everything on
>  * press rfkill button (expect -> hw_blocked transition)
>  * put interface down
>  * press rfkill button (expect -> unblocked transition)
>  * put interface up

I'm running v2.6.30-rc8-25743-g3b2029b from wireless-testing.

This is with interface UP and switch off:

larrylap:/etc/sysconfig/network # cat /sys/class/rfkill/rfkill0/uevent
PHYSDEVPATH=/devices/pci0000:00/0000:00:0d.0/0000:04:00.0/ssb0:0
PHYSDEVBUS=ssb
PHYSDEVDRIVER=b43
RFKILL_NAME=phy0
RFKILL_TYPE=wlan
RFKILL_STATE=2


larrylap:/etc/sysconfig/network # ifdown eth1
    eth1      name: BCM4312 802.11a/b/g
larrylap:/etc/sysconfig/network # cat /sys/class/rfkill/rfkill0/uevent
PHYSDEVPATH=/devices/pci0000:00/0000:00:0d.0/0000:04:00.0/ssb0:0
PHYSDEVBUS=ssb
PHYSDEVDRIVER=b43
RFKILL_NAME=phy0
RFKILL_TYPE=wlan
RFKILL_STATE=2

At this point, radio kill switch turned on:

larrylap:/etc/sysconfig/network # ifup eth1
    eth1      name: BCM4312 802.11a/b/g
    eth1      warning: WPA configured but may be unsupported
    eth1      warning: by this device
    eth1      starting wpa_supplicant
SIOCSIFFLAGS: Unknown error 132
Could not set interface 'eth1' UP
RTNETLINK answers: Unknown error 132
Starting DHCP4 client on eth1. . . .
    eth1      DHCP4 client NOT running
RTNETLINK answers: Unknown error 132
Cannot enable interface eth1.
interface eth1 is not up

larrylap:/etc/sysconfig/network # cat /sys/class/rfkill/rfkill0/uevent
PHYSDEVPATH=/devices/pci0000:00/0000:00:0d.0/0000:04:00.0/ssb0:0
PHYSDEVBUS=ssb
PHYSDEVDRIVER=b43
RFKILL_NAME=phy0
RFKILL_TYPE=wlan
RFKILL_STATE=2

The ERFKILL (132) prevents it from coming up. Again, the switch was on
at this point.

I could not restore service until rmmod/insmod of b43.

Larry

Re: [PATCH] rfkill: always init poll delayed work

[Johannes Berg]

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

On Thu, 2009-06-04 at 10:59 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> > 
> > Could you try, without the patch in question, what happens in terms of
> > uevents
> >  * have everything on
> >  * press rfkill button (expect -> hw_blocked transition)
> >  * put interface down
> >  * press rfkill button (expect -> unblocked transition)
> >  * put interface up
> 
> I'm running v2.6.30-rc8-25743-g3b2029b from wireless-testing.

But this is without the patch?! If yes that would confuse me.

johannes

> This is with interface UP and switch off:
> 
> larrylap:/etc/sysconfig/network # cat /sys/class/rfkill/rfkill0/uevent
> PHYSDEVPATH=/devices/pci0000:00/0000:00:0d.0/0000:04:00.0/ssb0:0
> PHYSDEVBUS=ssb
> PHYSDEVDRIVER=b43
> RFKILL_NAME=phy0
> RFKILL_TYPE=wlan
> RFKILL_STATE=2
> 
> 
> larrylap:/etc/sysconfig/network # ifdown eth1
>     eth1      name: BCM4312 802.11a/b/g
> larrylap:/etc/sysconfig/network # cat /sys/class/rfkill/rfkill0/uevent
> PHYSDEVPATH=/devices/pci0000:00/0000:00:0d.0/0000:04:00.0/ssb0:0
> PHYSDEVBUS=ssb
> PHYSDEVDRIVER=b43
> RFKILL_NAME=phy0
> RFKILL_TYPE=wlan
> RFKILL_STATE=2
> 
> At this point, radio kill switch turned on:
> 
> larrylap:/etc/sysconfig/network # ifup eth1
>     eth1      name: BCM4312 802.11a/b/g
>     eth1      warning: WPA configured but may be unsupported
>     eth1      warning: by this device
>     eth1      starting wpa_supplicant
> SIOCSIFFLAGS: Unknown error 132
> Could not set interface 'eth1' UP
> RTNETLINK answers: Unknown error 132
> Starting DHCP4 client on eth1. . . .
>     eth1      DHCP4 client NOT running
> RTNETLINK answers: Unknown error 132
> Cannot enable interface eth1.
> interface eth1 is not up
> 
> larrylap:/etc/sysconfig/network # cat /sys/class/rfkill/rfkill0/uevent
> PHYSDEVPATH=/devices/pci0000:00/0000:00:0d.0/0000:04:00.0/ssb0:0
> PHYSDEVBUS=ssb
> PHYSDEVDRIVER=b43
> RFKILL_NAME=phy0
> RFKILL_TYPE=wlan
> RFKILL_STATE=2
> 
> The ERFKILL (132) prevents it from coming up. Again, the switch was on
> at this point.
> 
> I could not restore service until rmmod/insmod of b43.
> 
> Larry
> 

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

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:

> But this is without the patch?! If yes that would confuse me.

My bad. That test was with the patch "[RFT 3/3] b43/legacy: port to
cfg80211 rfkill". Without it, everything works just fine. The LED
works and the radio follows the switch.

Sorry for the noise.

Now that I know the problem is in the patch, it should be easier to debug.

Larry

Re: [PATCH] rfkill: always init poll delayed work

[Johannes Berg]

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

On Thu, 2009-06-04 at 11:28 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> 
> > But this is without the patch?! If yes that would confuse me.
> 
> My bad. That test was with the patch "[RFT 3/3] b43/legacy: port to
> cfg80211 rfkill". Without it, everything works just fine. The LED
> works and the radio follows the switch.
> 
> Sorry for the noise.

No worries.

> Now that I know the problem is in the patch, it should be easier to debug.

Would you do the same test with the uevents w/o the patch? I suspect
that you won't get the event at the right spot.

johannes

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

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:
> 
> Would you do the same test with the uevents w/o the patch? I suspect
> that you won't get the event at the right spot.

Yes. Without the patch and the interface UP or DOWN,
/sys/class/rfkill/rfkill0/uevent always shows RFKILL_STATE=1, no
matter what state the switch is in.

FYI, my switch is a slider with off to the left and on to the right.

Larry

Re: [PATCH] rfkill: always init poll delayed work

[Johannes Berg]

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

On Thu, 2009-06-04 at 11:51 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> > 
> > Would you do the same test with the uevents w/o the patch? I suspect
> > that you won't get the event at the right spot.
> 
> Yes. Without the patch and the interface UP or DOWN,
> /sys/class/rfkill/rfkill0/uevent always shows RFKILL_STATE=1, no
> matter what state the switch is in.

Right. So we already have the problem in the old code, we just don't
notice much... can we take the core up, check, and put it down again? Or
I think it's GPIOs, can we query those via chipcommon when the 802.11
core is down?

> FYI, my switch is a slider with off to the left and on to the right.

ok.

johannes

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

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:
> 
> Right. So we already have the problem in the old code, we just don't
> notice much... can we take the core up, check, and put it down again? Or
> I think it's GPIOs, can we query those via chipcommon when the 802.11
> core is down?

If you mean the check for the switch, that is an MMIO access. I think
that we can access it even if the 802.11 core is down.

Larry

Re: [PATCH] rfkill: always init poll delayed work

[Johannes Berg]

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

On Thu, 2009-06-04 at 12:52 -0500, Larry Finger wrote:
> Johannes Berg wrote:
> > 
> > Right. So we already have the problem in the old code, we just don't
> > notice much... can we take the core up, check, and put it down again? Or
> > I think it's GPIOs, can we query those via chipcommon when the 802.11
> > core is down?
> 
> If you mean the check for the switch, that is an MMIO access. I think
> that we can access it even if the 802.11 core is down.

I do mean that, but I don't think you can access the 802.11 core's MMIO
while it's down. I'm out of ideas expect taking the core up, checking,
and down again periodically.

johannes

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

Re: [PATCH] rfkill: always init poll delayed work

[Larry Finger]

Johannes Berg wrote:
> 
> I do mean that, but I don't think you can access the 802.11 core's MMIO
> while it's down. I'm out of ideas expect taking the core up, checking,
> and down again periodically.

You were right. What I missed was that the stop callback was taking
the interface down to the uninitialized state, which caused the rfkill
poll routine to bail out without testing the switch state in the
hardware. A quick fix is for the poll routine to call
b43_wireless_core_init() whenever it detects the uninitialized state.
This call brings the interface back to the initialized state where the
 switch state can be tested. With this change, the radio now follows
the switch. The LED now stays on all the time - always something to fix.

What bothers me is that this seems like a lot of work when the same
effect could be achieved by simply turning the radio off/on when the
rfkill blocking state changes. Perhaps there should be the option of
using two new ieee80211_ops callbacks (radio_off/radio_on) that would
default to stop/start if not specified. By specifying these callbacks,
each driver could choose how intrusive the radio shutdown need be.

Larry