LinuxLists.cc - [RFC/RFT] rtl8187: Implement rfkill support

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, Aug 26, 2009 at 1:33 PM, Luis R. Rodriguez<[email protected]> wrote:
> On Wed, Aug 26, 2009 at 7:34 AM, Johannes Berg<[email protected]> wrote:
>> On Wed, 2009-08-26 at 13:33 +0000, Hin-Tak Leung wrote:
>>
>>> > Or wait ... are you using compat-wireless?
>>>
>>> Yes, I am. I mentioned this and did wonder if the _backport/ part
>>> in /sys/class is important.
>>
>> Sorry, didn't see.
>>
>> Anyway, that's pretty clearly the reason -- Luis added NETDEV_PRE_UP to
>> some compat*.h but obviously the kernel won't ever call that notifier,
>> so cfg80211 doesn't get a chance to reject the IFUP. No idea how to
>> handle that -- it'll be working fine in a regular tree.
>>
>> Luis, the only way to handle that would be to manually call the PRE_UP
>> notifier from mac80211's subif_open() and if that returns an error
>> (warning: the calling convention is weird) return the error... that's
>> weird but would work.
>
> Neat, thanks. Not sure if I'll get to this this anytime soon, but if
> someone is interested in it please give it a shot and send a patch.
> That would be nice.

What if we *also* just do that from mac80211 as well upstream?

Luis

2009-08-26 16:57:08

by Herton Ronaldo Krzesinski

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Em Qua 26 Ago 2009, ?s 12:07:52, Hin-Tak Leung escreveu:
> On Wed, Aug 26, 2009 at 3:34 PM, Johannes Berg<[email protected]> wrote:
> > On Wed, 2009-08-26 at 13:33 +0000, Hin-Tak Leung wrote:
> >
> >> > Or wait ... are you using compat-wireless?
> >>
> >> Yes, I am. I mentioned this and did wonder if the _backport/ part
> >> in /sys/class is important.
> >
> > Sorry, didn't see.
> >
> > Anyway, that's pretty clearly the reason -- Luis added NETDEV_PRE_UP to
> > some compat*.h but obviously the kernel won't ever call that notifier,
> > so cfg80211 doesn't get a chance to reject the IFUP. No idea how to
> > handle that -- it'll be working fine in a regular tree.
> >
> > Luis, the only way to handle that would be to manually call the PRE_UP
> > notifier from mac80211's subif_open() and if that returns an error
> > (warning: the calling convention is weird) return the error... that's
> > weird but would work.
>
> Okay, that explains it. So I can have a Tested-by: ... I just grep for

Ok, I submitted now the patches.

> NETDEV_PRE_UP in compat-wireless and it is only in
> include/net/compat-2.6.31.h (not in 2.6.30) and I am on 2.6.30.5-X . I
> can grab the rawhide 2.6.31 rpms and try it quickly;
> and possibly look at some ugly quick hack backport that? Stay tuned.
>
> Thanks for the help.
>
> Hin-Tak
>

--
[]'s
Herton

2009-08-25 06:57:58

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

--- On Tue, 25/8/09, Larry Finger <[email protected]> wrote:

> Hin-Tak Leung wrote:
> > --- On Tue, 25/8/09, Larry Finger <[email protected]>
> wrote:
> >
> >>> [PATCH] rtl8187: fix circular locking
> >> (rtl8187_stop/rtl8187_work)
> >>
> >> This patch fixes the problem. You can add a
> Tested-by to
> >> it.
> >
> > Hmm, I am still wondering about why NM insists on if
> up'ing the device. I read bits of things and apparently hal
> is supposed to know the device is rfkill'ed and let NM know.
> But lshal is not listing the device as having an killswitch.
> I don't know how hal is supposed to work out that info
> though.
> >
> > also I noted that
> /sys/class/rfkill_backport/rfkill0/state goes from 1 to 2
> when I slide the switch to the 'off' position. Some says it
> should be 0?
> > Don't know if hal is affected by its being
> rfkill_backport (compat-wireless) rather than rfkill (stock
> vendor kernel). well, it should look there if it isn't :-).
> >
> > It looks like it is a hal problem...
>
> The interpretation is as follows:
> 0 - blocked by software such as 'rfkill block 1'
> 1 - unblocked
> 2 - blocked by hardware
>
> Your state is doing exactly what I would expect. When NM
> brings the
> device up, does the above state change? Does dmesg show
> anything?

dmesg and rfkill/state corresponds exactly to what the sliding switch does (in the on position, dmesg says it is on, states says 2, in the off position, dmesg says it is off, state says 1). NM basically ignores rfkill/state, and just if'up the device whenever it notices the device has gone down.

hal is supposed to have knowledge of the rfkill state (which it hasn't, from what I see of the lshal output - there is no killswitch entry in there), and let NM through dbus know to not bother the device. At the moment I think it is somewhere in hal that's not working properly.

2009-08-26 13:33:39

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

--- On Wed, 26/8/09, Johannes Berg <[email protected]> wrote:

> On Wed, 2009-08-26 at 03:43 +0100,
> Hin-Tak Leung wrote:
>
> > The rfkill event hard just goes 1 and 0 whenever I
> slide the switch,
> > regardless of what NM does . e.g. NM set to disable
> networking has no
> > effect on the 1/0 switch (it happens just depending on
> the slide
> > switch and nothing else), and nothing changes when NM
> noticed
> > rfkill'ed if down & decided to if'up and
> reassociate.
>
> Right, I just wanted to make sure the switch could be
> correctly read
> while the interface is down.
>
> > Basically the rfkill event state just depends on the
> slide switch,
> > regardless of NM (if it is set to enable wireless
> networking, it just
> > if up the device again; if it is set to disable, it
> just stayed
> > disable & the device stay down, but the event
> state continues to
> > respond to the slide switch).
>
> Ok. But this isn't making a lot of sense to me, since
> cfg80211 should
> refuse to UP the device when it's rfkilled.
>
> Or wait ... are you using compat-wireless?

Yes, I am. I mentioned this and did wonder if the _backport/ part in /sys/class is important.

Hin-Tak

2009-08-25 09:28:42

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Tue, 2009-08-25 at 06:51 +0000, Hin-Tak Leung wrote:

> dmesg and rfkill/state corresponds exactly to what the sliding switch
> does (in the on position, dmesg says it is on, states says 2, in the
> off position, dmesg says it is off, state says 1). NM basically
> ignores rfkill/state, and just if'up the device whenever it notices
> the device has gone down.

NM can't ignore the state since cfg80211 enforces it.

Can you please use rfkill (http://git.sipsolutions.net/rfkill.git) to
see what's going on? Like print out events while you slide the button
and use NM etc.

One thing that would be possible -- does your poll callback work while
the interface is down? Try to tell NM to turn off the wireless network,
and see whether the state ever changes with your slider.

johannes

Attachments:

signature.asc (801.00 B)
This is a digitally signed message part

2009-08-26 02:43:38

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Tue, Aug 25, 2009 at 10:27 AM, Johannes
Berg<[email protected]> wrote:
> On Tue, 2009-08-25 at 06:51 +0000, Hin-Tak Leung wrote:
>
>> dmesg and rfkill/state corresponds exactly to what the sliding switch
>> does (in the on position, dmesg says it is on, states says 2, in the
>> off position, dmesg says it is off, state says 1). NM basically
>> ignores rfkill/state, and just if'up the device whenever it notices
>> the device has gone down.
>
> NM can't ignore the state since cfg80211 enforces it.
>
> Can you please use rfkill (http://git.sipsolutions.net/rfkill.git) to
> see what's going on? Like print out events while you slide the button
> and use NM etc.
>
> One thing that would be possible -- does your poll callback work while
> the interface is down? Try to tell NM to turn off the wireless network,
> and see whether the state ever changes with your slider.

--------------------
[root@localhost rfkill]# ./rfkill list
0: (null): Wireless LAN
Soft blocked: no
Hard blocked: no
[root@localhost rfkill]# ./rfkill event
RFKILL event: idx 0 type 1 op 0 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
RFKILL event: idx 0 type 1 op 2 soft 0 hard 0
RFKILL event: idx 0 type 1 op 2 soft 0 hard 1
^C
[root@localhost rfkill]# ./rfkill list
0: (null): Wireless LAN
Soft blocked: no
Hard blocked: yes
[root@localhost rfkill]# ./rfkill list
0: (null): Wireless LAN
Soft blocked: no
Hard blocked: no
--------------
The rfkill event hard just goes 1 and 0 whenever I slide the switch,
regardless of what NM does . e.g. NM set to disable networking has no
effect on the 1/0 switch (it happens just depending on the slide
switch and nothing else), and nothing changes when NM noticed
rfkill'ed if down & decided to if'up and reassociate.

Basically the rfkill event state just depends on the slide switch,
regardless of NM (if it is set to enable wireless networking, it just
if up the device again; if it is set to disable, it just stayed
disable & the device stay down, but the event state continues to
respond to the slide switch).

2009-08-25 02:57:27

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

--- On Tue, 25/8/09, Larry Finger <[email protected]> wrote:

> > [PATCH] rtl8187: fix circular locking
> (rtl8187_stop/rtl8187_work)
>
> This patch fixes the problem. You can add a Tested-by to
> it.

Hmm, I am still wondering about why NM insists on if up'ing the device. I read bits of things and apparently hal is supposed to know the device is rfkill'ed and let NM know. But lshal is not listing the device as having an killswitch. I don't know how hal is supposed to work out that info though.

also I noted that /sys/class/rfkill_backport/rfkill0/state goes from 1 to 2 when I slide the switch to the 'off' position. Some says it should be 0?
Don't know if hal is affected by its being rfkill_backport (compat-wireless) rather than rfkill (stock vendor kernel). well, it should look there if it isn't :-).

It looks like it is a hal problem...

2009-08-24 21:03:34

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Mon, Aug 24, 2009 at 7:10 PM, Herton Ronaldo
Krzesinski<[email protected]> wrote:
<snipped>
> This is strange, may be NetworkManager does something extra and for some reason
> manages to wake up the interface again? I thought it would be unable to "if up"
> the interface since -ERFKILL should be returned to it when switch is off...
>
>>
>> Is this intentional? I thought it is supposed to actually switch off
>> the hardware, or is this how the code supposed to work?
>> Maybe some component needs to let NM knows not to reenanble the device.
>
> About taking interface down, thats expected, but I don't know about
> NetworkManager, I was expecting it to be unable to "if up" the interface again.

Is there a minimum version of NM/wpa_supplicant this is expected? I
could upgrade if needed to. I am mostly fedora 11 based and have NM
0.7.1-8.git20090708 , wpa_supplicant 0.6.8.

Hin-Tak

2009-08-24 18:10:09

by Herton Ronaldo Krzesinski

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Em Dom 23 Ago 2009, ?s 16:38:43, Hin-Tak Leung escreveu:
> On Sat, Aug 22, 2009 at 10:59 PM, Hin-Tak Leung<[email protected]> wrote:
> > On Sat, Aug 22, 2009 at 4:38 AM, Herton Ronaldo
> > Krzesinski<[email protected]> wrote:
> >> This change implements rfkill support for RTL8187B and RTL8187L devices,
> >> using new cfg80211 rfkill API.
> >>
> >> Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>
> >
> > Tested-by: Hin-Tak Leung <[email protected]>
> >
> > Neat!
> >
> > I ran a ping to the router while flicking the switch on and off, and
> > have 'ping: sendmsg: Network is unreachable' in the middle. dmesg also
> > shows
>
> It appears that I wrote prematurely - flicking the switch with the
> patch seems to just do the equivalent of ifconfig down.

Yes, the equivalent of ifconfig down is what cfg80211 rfkill does in this case.

> (before the patch, the switch has no effect). However, NetworkManager
> soon notices the interface going down and ifup it again. So I have
> simply got a 30s to 1 minute disruption in connectivity.

This is strange, may be NetworkManager does something extra and for some reason
manages to wake up the interface again? I thought it would be unable to "if up"
the interface since -ERFKILL should be returned to it when switch is off...

>
> Is this intentional? I thought it is supposed to actually switch off
> the hardware, or is this how the code supposed to work?
> Maybe some component needs to let NM knows not to reenanble the device.

About taking interface down, thats expected, but I don't know about
NetworkManager, I was expecting it to be unable to "if up" the interface again.

>
> Hin-Tak
>

--
[]'s
Herton

2009-08-26 16:29:25

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, Aug 26, 2009 at 3:34 PM, Johannes Berg<[email protected]> wrote:
> On Wed, 2009-08-26 at 13:33 +0000, Hin-Tak Leung wrote:
>
>> > Or wait ... are you using compat-wireless?
>>
>> Yes, I am. I mentioned this and did wonder if the _backport/ part
>> in /sys/class is important.
>
> Sorry, didn't see.
>
> Anyway, that's pretty clearly the reason -- Luis added NETDEV_PRE_UP to
> some compat*.h but obviously the kernel won't ever call that notifier,
> so cfg80211 doesn't get a chance to reject the IFUP. No idea how to
> handle that -- it'll be working fine in a regular tree.
>
> Luis, the only way to handle that would be to manually call the PRE_UP
> notifier from mac80211's subif_open() and if that returns an error
> (warning: the calling convention is weird) return the error... that's
> weird but would work.
>
> johannes
>

Hmm, got a bit side-tracked. But hal doesn't know the device having a
killswitch is still wrong somewhere?
(i.e. am wondering where we should advertise that ability, or how hal
works that out)

Hin-Tak

2009-08-26 08:58:26

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, 2009-08-26 at 03:43 +0100, Hin-Tak Leung wrote:

> The rfkill event hard just goes 1 and 0 whenever I slide the switch,
> regardless of what NM does . e.g. NM set to disable networking has no
> effect on the 1/0 switch (it happens just depending on the slide
> switch and nothing else), and nothing changes when NM noticed
> rfkill'ed if down & decided to if'up and reassociate.

Right, I just wanted to make sure the switch could be correctly read
while the interface is down.

> Basically the rfkill event state just depends on the slide switch,
> regardless of NM (if it is set to enable wireless networking, it just
> if up the device again; if it is set to disable, it just stayed
> disable & the device stay down, but the event state continues to
> respond to the slide switch).

Ok. But this isn't making a lot of sense to me, since cfg80211 should
refuse to UP the device when it's rfkilled.

Or wait ... are you using compat-wireless?

johannes

Attachments:

signature.asc (801.00 B)
This is a digitally signed message part

2009-08-22 17:13:59

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Herton Ronaldo Krzesinski wrote:
> This change implements rfkill support for RTL8187B and RTL8187L devices,
> using new cfg80211 rfkill API.
>
> Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>
> ---

I don't have an RFKILL switch on my rtl8187 devices and cannot test
that part; however, everything looks OK and the devices still work.

Acked-by: Larry Finger <[email protected]>

Larry

2009-08-22 21:59:32

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Sat, Aug 22, 2009 at 4:38 AM, Herton Ronaldo
Krzesinski<[email protected]> wrote:
> This change implements rfkill support for RTL8187B and RTL8187L devices,
> using new cfg80211 rfkill API.
>
> Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>

Tested-by: Hin-Tak Leung <[email protected]>

Neat!

I ran a ping to the router while flicking the switch on and off, and
have 'ping: sendmsg: Network is unreachable' in the middle. dmesg also
shows

rtl8187: wireless radio switch turned off
wlan2: deauthenticating by local choice (reason=3)
rtl8187: wireless radio switch turned on

etc. Work as advertised.

Well done!

Hin-Tak

2009-08-25 03:37:33

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Hin-Tak Leung wrote:
> --- On Tue, 25/8/09, Larry Finger <[email protected]> wrote:
>
>>> [PATCH] rtl8187: fix circular locking
>> (rtl8187_stop/rtl8187_work)
>>
>> This patch fixes the problem. You can add a Tested-by to
>> it.
>
> Hmm, I am still wondering about why NM insists on if up'ing the device. I read bits of things and apparently hal is supposed to know the device is rfkill'ed and let NM know. But lshal is not listing the device as having an killswitch. I don't know how hal is supposed to work out that info though.
>
> also I noted that /sys/class/rfkill_backport/rfkill0/state goes from 1 to 2 when I slide the switch to the 'off' position. Some says it should be 0?
> Don't know if hal is affected by its being rfkill_backport (compat-wireless) rather than rfkill (stock vendor kernel). well, it should look there if it isn't :-).
>
> It looks like it is a hal problem...

The interpretation is as follows:
0 - blocked by software such as 'rfkill block 1'
1 - unblocked
2 - blocked by hardware

Your state is doing exactly what I would expect. When NM brings the
device up, does the above state change? Does dmesg show anything?

Larry

2009-08-26 20:33:54

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, Aug 26, 2009 at 7:34 AM, Johannes Berg<[email protected]> wrote:
> On Wed, 2009-08-26 at 13:33 +0000, Hin-Tak Leung wrote:
>
>> > Or wait ... are you using compat-wireless?
>>
>> Yes, I am. I mentioned this and did wonder if the _backport/ part
>> in /sys/class is important.
>
> Sorry, didn't see.
>
> Anyway, that's pretty clearly the reason -- Luis added NETDEV_PRE_UP to
> some compat*.h but obviously the kernel won't ever call that notifier,
> so cfg80211 doesn't get a chance to reject the IFUP. No idea how to
> handle that -- it'll be working fine in a regular tree.
>
> Luis, the only way to handle that would be to manually call the PRE_UP
> notifier from mac80211's subif_open() and if that returns an error
> (warning: the calling convention is weird) return the error... that's
> weird but would work.

Neat, thanks. Not sure if I'll get to this this anytime soon, but if
someone is interested in it please give it a shot and send a patch.
That would be nice.

Luis

2009-08-26 22:31:02

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, 2009-08-26 at 15:26 -0700, Luis R. Rodriguez wrote:

> I was thinking of the case where someone hit rfkill between dev_open()
> and mac80211's subif_open() but I am either not sure if this is
> possible.

Oh, but that's ok -- we'll set the interface down again very quickly
after. Besides, you could be in subif_open() already while hitting it,
somewhere in driver initialisation...

> If there is a real value to adding it to mac80211 then we wouldn't
> need to tug around a hunk for it on compat.

I don't see any value in it.

johannes

Attachments:

signature.asc (801.00 B)
This is a digitally signed message part

2009-08-26 12:47:10

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Johannes Berg wrote:
> On Wed, 2009-08-26 at 03:43 +0100, Hin-Tak Leung wrote:
>
>> The rfkill event hard just goes 1 and 0 whenever I slide the switch,
>> regardless of what NM does . e.g. NM set to disable networking has no
>> effect on the 1/0 switch (it happens just depending on the slide
>> switch and nothing else), and nothing changes when NM noticed
>> rfkill'ed if down & decided to if'up and reassociate.
>
> Right, I just wanted to make sure the switch could be correctly read
> while the interface is down.
>
>> Basically the rfkill event state just depends on the slide switch,
>> regardless of NM (if it is set to enable wireless networking, it just
>> if up the device again; if it is set to disable, it just stayed
>> disable & the device stay down, but the event state continues to
>> respond to the slide switch).
>
> Ok. But this isn't making a lot of sense to me, since cfg80211 should
> refuse to UP the device when it's rfkilled.
>
> Or wait ... are you using compat-wireless?

Yes he is.

2009-08-24 18:03:40

by Herton Ronaldo Krzesinski

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Em Dom 23 Ago 2009, ?s 22:46:40, Larry Finger escreveu:
> Herton Ronaldo Krzesinski wrote:
> > This change implements rfkill support for RTL8187B and RTL8187L devices,
> > using new cfg80211 rfkill API.
> >
> > Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>
> > ---
>
> I found a problem with this patch. When I issue an 'rfkill block 1'
> command, I get the following circular locking warning:

Hmm, this is a issue that was already present before the rfkill patch, but
seems with it, it became more likely to happen. Please try this patch:

[PATCH] rtl8187: fix circular locking (rtl8187_stop/rtl8187_work)

Larry Finger reports following lockdep warning:

[ INFO: possible circular locking dependency detected ]
2.6.31-rc6-wl #201
-------------------------------------------------------
rfkill/30578 is trying to acquire lock:
(&(&priv->work)->work#2){+.+...}, at: [<ffffffff81051215>]
__cancel_work_timer+0xd9/0x222

but task is already holding lock:
(&priv->conf_mutex#2){+.+.+.}, at: [<ffffffffa064a024>]
rtl8187_stop+0x31/0x364 [rtl8187]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&priv->conf_mutex#2){+.+.+.}:
[<ffffffff81065957>] __lock_acquire+0x12d0/0x1614
[<ffffffff81065d54>] lock_acquire+0xb9/0xdd
[<ffffffff8127c32f>] mutex_lock_nested+0x56/0x2a8
[<ffffffffa064a392>] rtl8187_work+0x3b/0xf2 [rtl8187]
[<ffffffff81050758>] worker_thread+0x1fa/0x30a
[<ffffffff81054ca5>] kthread+0x8f/0x97
[<ffffffff8100cb7a>] child_rip+0xa/0x20
[<ffffffffffffffff>] 0xffffffffffffffff

-> #0 (&(&priv->work)->work#2){+.+...}:
[<ffffffff8106568c>] __lock_acquire+0x1005/0x1614
[<ffffffff81065d54>] lock_acquire+0xb9/0xdd
[<ffffffff8105124e>] __cancel_work_timer+0x112/0x222
[<ffffffff8105136b>] cancel_delayed_work_sync+0xd/0xf
[<ffffffffa064a33f>] rtl8187_stop+0x34c/0x364 [rtl8187]
[<ffffffffa0242866>] ieee80211_stop_device+0x29/0x61 [mac80211]
[<ffffffffa0239194>] ieee80211_stop+0x476/0x530 [mac80211]
[<ffffffff8120ce15>] dev_close+0x8a/0xac
[<ffffffffa01d9fa7>] cfg80211_rfkill_set_block+0x4a/0x7a [cfg80211]
[<ffffffffa01bf4f0>] rfkill_set_block+0x84/0xd9 [rfkill]
[<ffffffffa01bfc31>] rfkill_fop_write+0xda/0x124 [rfkill]
[<ffffffff810cf286>] vfs_write+0xae/0x14a
[<ffffffff810cf3e6>] sys_write+0x47/0x6e
[<ffffffff8100ba6b>] system_call_fastpath+0x16/0x1b
[<ffffffffffffffff>] 0xffffffffffffffff

The problem here is that rtl8187_stop, while helding priv->conf_mutex,
runs cancel_delayed_work_sync on an workqueue that runs rtl8187_work,
which also takes priv->conf_mutex lock. Move cancel_delayed_work_sync
out of rtl8187_stop priv->conf_mutex locking region.

Reported-by: Larry Finger <[email protected]>
Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8187_dev.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index b6e9fbd..2017ccc 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1018,9 +1018,10 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
dev_kfree_skb_any(skb);

usb_kill_anchored_urbs(&priv->anchored);
+ mutex_unlock(&priv->conf_mutex);
+
if (!priv->is_rtl8187b)
cancel_delayed_work_sync(&priv->work);
- mutex_unlock(&priv->conf_mutex);
}

static int rtl8187_add_interface(struct ieee80211_hw *dev,
--
1.6.4.1

>
>
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.31-rc6-wl #201
> -------------------------------------------------------
> rfkill/30578 is trying to acquire lock:
> (&(&priv->work)->work#2){+.+...}, at: [<ffffffff81051215>]
> __cancel_work_timer+0xd9/0x222
>
> but task is already holding lock:
> (&priv->conf_mutex#2){+.+.+.}, at: [<ffffffffa064a024>]
> rtl8187_stop+0x31/0x364 [rtl8187]
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&priv->conf_mutex#2){+.+.+.}:
> [<ffffffff81065957>] __lock_acquire+0x12d0/0x1614
> [<ffffffff81065d54>] lock_acquire+0xb9/0xdd
> [<ffffffff8127c32f>] mutex_lock_nested+0x56/0x2a8
> [<ffffffffa064a392>] rtl8187_work+0x3b/0xf2 [rtl8187]
> [<ffffffff81050758>] worker_thread+0x1fa/0x30a
> [<ffffffff81054ca5>] kthread+0x8f/0x97
> [<ffffffff8100cb7a>] child_rip+0xa/0x20
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> -> #0 (&(&priv->work)->work#2){+.+...}:
> [<ffffffff8106568c>] __lock_acquire+0x1005/0x1614
> [<ffffffff81065d54>] lock_acquire+0xb9/0xdd
> [<ffffffff8105124e>] __cancel_work_timer+0x112/0x222
> [<ffffffff8105136b>] cancel_delayed_work_sync+0xd/0xf
> [<ffffffffa064a33f>] rtl8187_stop+0x34c/0x364 [rtl8187]
> [<ffffffffa0242866>] ieee80211_stop_device+0x29/0x61 [mac80211]
> [<ffffffffa0239194>] ieee80211_stop+0x476/0x530 [mac80211]
> [<ffffffff8120ce15>] dev_close+0x8a/0xac
> [<ffffffffa01d9fa7>] cfg80211_rfkill_set_block+0x4a/0x7a [cfg80211]
> [<ffffffffa01bf4f0>] rfkill_set_block+0x84/0xd9 [rfkill]
> [<ffffffffa01bfc31>] rfkill_fop_write+0xda/0x124 [rfkill]
> [<ffffffff810cf286>] vfs_write+0xae/0x14a
> [<ffffffff810cf3e6>] sys_write+0x47/0x6e
> [<ffffffff8100ba6b>] system_call_fastpath+0x16/0x1b
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> other info that might help us debug this:
>
> 4 locks held by rfkill/30578:
> #0: (rfkill_global_mutex){+.+.+.}, at: [<ffffffffa01bfbbc>]
> rfkill_fop_write+0x65/0x124 [rfkill]
> #1: (rtnl_mutex){+.+.+.}, at: [<ffffffff81215a60>] rtnl_lock+0x12/0x14
> #2: (&rdev->devlist_mtx){+.+.+.}, at: [<ffffffffa01d9f89>]
> cfg80211_rfkill_set_block+0x2c/0x7a [cfg80211]
> #3: (&priv->conf_mutex#2){+.+.+.}, at: [<ffffffffa064a024>]
> rtl8187_stop+0x31/0x364 [rtl8187]
>
> stack backtrace:
> Pid: 30578, comm: rfkill Not tainted 2.6.31-rc6-wl #201
> Call Trace:
> [<ffffffff810641ec>] print_circular_bug_tail+0xc1/0xcc
> [<ffffffff8106568c>] __lock_acquire+0x1005/0x1614
> [<ffffffff81065d54>] lock_acquire+0xb9/0xdd
> [<ffffffff81051215>] ? __cancel_work_timer+0xd9/0x222
> [<ffffffff8105124e>] __cancel_work_timer+0x112/0x222
> [<ffffffff81051215>] ? __cancel_work_timer+0xd9/0x222
> [<ffffffff81063791>] ? mark_held_locks+0x4d/0x6b
> [<ffffffff8127db94>] ? _spin_unlock_irq+0x2b/0x30
> [<ffffffff81063a04>] ? trace_hardirqs_on_caller+0x10b/0x12f
> [<ffffffff81063a35>] ? trace_hardirqs_on+0xd/0xf
> [<ffffffff8127db94>] ? _spin_unlock_irq+0x2b/0x30
> [<ffffffffa00de2c4>] ? usb_kill_anchored_urbs+0x46/0x5c [usbcore]
> [<ffffffff8105136b>] cancel_delayed_work_sync+0xd/0xf
> [<ffffffffa064a33f>] rtl8187_stop+0x34c/0x364 [rtl8187]
> [<ffffffffa0242866>] ieee80211_stop_device+0x29/0x61 [mac80211]
> [<ffffffffa0239194>] ieee80211_stop+0x476/0x530 [mac80211]
> [<ffffffffa0238d68>] ? ieee80211_stop+0x4a/0x530 [mac80211]
> [<ffffffff81044a28>] ? local_bh_enable_ip+0xc8/0xcd
> [<ffffffff8127db65>] ? _spin_unlock_bh+0x2f/0x33
> [<ffffffff8121d116>] ? dev_deactivate+0x162/0x192
> [<ffffffff8120ce15>] dev_close+0x8a/0xac
> [<ffffffffa01d9fa7>] cfg80211_rfkill_set_block+0x4a/0x7a [cfg80211]
> [<ffffffffa01bf4f0>] rfkill_set_block+0x84/0xd9 [rfkill]
> [<ffffffffa01bfc31>] rfkill_fop_write+0xda/0x124 [rfkill]
> [<ffffffff8100ba9c>] ? sysret_check+0x27/0x62
> [<ffffffff810cf286>] vfs_write+0xae/0x14a
> [<ffffffff810cf3e6>] sys_write+0x47/0x6e
> [<ffffffff8100ba6b>] system_call_fastpath+0x16/0x1bf
>
>
> Larry

--
[]'s
Herton

2009-08-26 22:26:24

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, Aug 26, 2009 at 3:12 PM, Johannes Berg<[email protected]> wrote:
> On Wed, 2009-08-26 at 15:06 -0700, Luis R. Rodriguez wrote:
>
>> > Neat, thanks. Not sure if I'll get to this this anytime soon, but if
>> > someone is interested in it please give it a shot and send a patch.
>> > That would be nice.
>>
>> What if we *also* just do that from mac80211 as well upstream?
>
> Why?

I was thinking of the case where someone hit rfkill between dev_open()
and mac80211's subif_open() but I am either not sure if this is
possible.

> It's fine as it is, and we don't want to clutter all cfg80211
> drivers with it.

If there is a real value to adding it to mac80211 then we wouldn't
need to tug around a hunk for it on compat.

Luis

2009-08-24 01:47:59

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

Herton Ronaldo Krzesinski wrote:
> This change implements rfkill support for RTL8187B and RTL8187L devices,
> using new cfg80211 rfkill API.
>
> Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>
> ---

I found a problem with this patch. When I issue an 'rfkill block 1'
command, I get the following circular locking warning:

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.31-rc6-wl #201
-------------------------------------------------------
rfkill/30578 is trying to acquire lock:
(&(&priv->work)->work#2){+.+...}, at: [<ffffffff81051215>]
__cancel_work_timer+0xd9/0x222

but task is already holding lock:
(&priv->conf_mutex#2){+.+.+.}, at: [<ffffffffa064a024>]
rtl8187_stop+0x31/0x364 [rtl8187]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&priv->conf_mutex#2){+.+.+.}:
[<ffffffff81065957>] __lock_acquire+0x12d0/0x1614
[<ffffffff81065d54>] lock_acquire+0xb9/0xdd
[<ffffffff8127c32f>] mutex_lock_nested+0x56/0x2a8
[<ffffffffa064a392>] rtl8187_work+0x3b/0xf2 [rtl8187]
[<ffffffff81050758>] worker_thread+0x1fa/0x30a
[<ffffffff81054ca5>] kthread+0x8f/0x97
[<ffffffff8100cb7a>] child_rip+0xa/0x20
[<ffffffffffffffff>] 0xffffffffffffffff

-> #0 (&(&priv->work)->work#2){+.+...}:
[<ffffffff8106568c>] __lock_acquire+0x1005/0x1614
[<ffffffff81065d54>] lock_acquire+0xb9/0xdd
[<ffffffff8105124e>] __cancel_work_timer+0x112/0x222
[<ffffffff8105136b>] cancel_delayed_work_sync+0xd/0xf
[<ffffffffa064a33f>] rtl8187_stop+0x34c/0x364 [rtl8187]
[<ffffffffa0242866>] ieee80211_stop_device+0x29/0x61 [mac80211]
[<ffffffffa0239194>] ieee80211_stop+0x476/0x530 [mac80211]
[<ffffffff8120ce15>] dev_close+0x8a/0xac
[<ffffffffa01d9fa7>] cfg80211_rfkill_set_block+0x4a/0x7a [cfg80211]
[<ffffffffa01bf4f0>] rfkill_set_block+0x84/0xd9 [rfkill]
[<ffffffffa01bfc31>] rfkill_fop_write+0xda/0x124 [rfkill]
[<ffffffff810cf286>] vfs_write+0xae/0x14a
[<ffffffff810cf3e6>] sys_write+0x47/0x6e
[<ffffffff8100ba6b>] system_call_fastpath+0x16/0x1b
[<ffffffffffffffff>] 0xffffffffffffffff

other info that might help us debug this:

4 locks held by rfkill/30578:
#0: (rfkill_global_mutex){+.+.+.}, at: [<ffffffffa01bfbbc>]
rfkill_fop_write+0x65/0x124 [rfkill]
#1: (rtnl_mutex){+.+.+.}, at: [<ffffffff81215a60>] rtnl_lock+0x12/0x14
#2: (&rdev->devlist_mtx){+.+.+.}, at: [<ffffffffa01d9f89>]
cfg80211_rfkill_set_block+0x2c/0x7a [cfg80211]
#3: (&priv->conf_mutex#2){+.+.+.}, at: [<ffffffffa064a024>]
rtl8187_stop+0x31/0x364 [rtl8187]

stack backtrace:
Pid: 30578, comm: rfkill Not tainted 2.6.31-rc6-wl #201
Call Trace:
[<ffffffff810641ec>] print_circular_bug_tail+0xc1/0xcc
[<ffffffff8106568c>] __lock_acquire+0x1005/0x1614
[<ffffffff81065d54>] lock_acquire+0xb9/0xdd
[<ffffffff81051215>] ? __cancel_work_timer+0xd9/0x222
[<ffffffff8105124e>] __cancel_work_timer+0x112/0x222
[<ffffffff81051215>] ? __cancel_work_timer+0xd9/0x222
[<ffffffff81063791>] ? mark_held_locks+0x4d/0x6b
[<ffffffff8127db94>] ? _spin_unlock_irq+0x2b/0x30
[<ffffffff81063a04>] ? trace_hardirqs_on_caller+0x10b/0x12f
[<ffffffff81063a35>] ? trace_hardirqs_on+0xd/0xf
[<ffffffff8127db94>] ? _spin_unlock_irq+0x2b/0x30
[<ffffffffa00de2c4>] ? usb_kill_anchored_urbs+0x46/0x5c [usbcore]
[<ffffffff8105136b>] cancel_delayed_work_sync+0xd/0xf
[<ffffffffa064a33f>] rtl8187_stop+0x34c/0x364 [rtl8187]
[<ffffffffa0242866>] ieee80211_stop_device+0x29/0x61 [mac80211]
[<ffffffffa0239194>] ieee80211_stop+0x476/0x530 [mac80211]
[<ffffffffa0238d68>] ? ieee80211_stop+0x4a/0x530 [mac80211]
[<ffffffff81044a28>] ? local_bh_enable_ip+0xc8/0xcd
[<ffffffff8127db65>] ? _spin_unlock_bh+0x2f/0x33
[<ffffffff8121d116>] ? dev_deactivate+0x162/0x192
[<ffffffff8120ce15>] dev_close+0x8a/0xac
[<ffffffffa01d9fa7>] cfg80211_rfkill_set_block+0x4a/0x7a [cfg80211]
[<ffffffffa01bf4f0>] rfkill_set_block+0x84/0xd9 [rfkill]
[<ffffffffa01bfc31>] rfkill_fop_write+0xda/0x124 [rfkill]
[<ffffffff8100ba9c>] ? sysret_check+0x27/0x62
[<ffffffff810cf286>] vfs_write+0xae/0x14a
[<ffffffff810cf3e6>] sys_write+0x47/0x6e
[<ffffffff8100ba6b>] system_call_fastpath+0x16/0x1bf

Larry

2009-08-26 14:35:22

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, 2009-08-26 at 13:33 +0000, Hin-Tak Leung wrote:

> > Or wait ... are you using compat-wireless?
>
> Yes, I am. I mentioned this and did wonder if the _backport/ part
> in /sys/class is important.

Sorry, didn't see.

Anyway, that's pretty clearly the reason -- Luis added NETDEV_PRE_UP to
some compat*.h but obviously the kernel won't ever call that notifier,
so cfg80211 doesn't get a chance to reject the IFUP. No idea how to
handle that -- it'll be working fine in a regular tree.

Luis, the only way to handle that would be to manually call the PRE_UP
notifier from mac80211's subif_open() and if that returns an error
(warning: the calling convention is weird) return the error... that's
weird but would work.

johannes

Attachments:

signature.asc (801.00 B)
This is a digitally signed message part

2009-08-23 19:38:44

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Sat, Aug 22, 2009 at 10:59 PM, Hin-Tak Leung<[email protected]> wrote:
> On Sat, Aug 22, 2009 at 4:38 AM, Herton Ronaldo
> Krzesinski<[email protected]> wrote:
>> This change implements rfkill support for RTL8187B and RTL8187L devices,
>> using new cfg80211 rfkill API.
>>
>> Signed-off-by: Herton Ronaldo Krzesinski <[email protected]>
>
> Tested-by: Hin-Tak Leung <[email protected]>
>
> Neat!
>
> I ran a ping to the router while flicking the switch on and off, and
> have 'ping: sendmsg: Network is unreachable' in the middle. dmesg also
> shows

It appears that I wrote prematurely - flicking the switch with the
patch seems to just do the equivalent of ifconfig down.
(before the patch, the switch has no effect). However, NetworkManager
soon notices the interface going down and ifup it again. So I have
simply got a 30s to 1 minute disruption in connectivity.

Is this intentional? I thought it is supposed to actually switch off
the hardware, or is this how the code supposed to work?
Maybe some component needs to let NM knows not to reenanble the device.

Hin-Tak

2009-08-26 22:47:09

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, Aug 26, 2009 at 3:30 PM, Johannes Berg<[email protected]> wrote:
> On Wed, 2009-08-26 at 15:26 -0700, Luis R. Rodriguez wrote:
>
>> I was thinking of the case where someone hit rfkill between dev_open()
>> and mac80211's subif_open() but I am either not sure if this is
>> possible.
>
> Oh, but that's ok -- we'll set the interface down again very quickly
> after. Besides, you could be in subif_open() already while hitting it,
> somewhere in driver initialisation...
>
>> If there is a real value to adding it to mac80211 then we wouldn't
>> need to tug around a hunk for it on compat.
>
> I don't see any value in it.

OK -- then someone interested can add the hunk to compat.

Luis

2009-08-26 15:14:49

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, Aug 26, 2009 at 3:34 PM, Johannes Berg<[email protected]> wrote:
> On Wed, 2009-08-26 at 13:33 +0000, Hin-Tak Leung wrote:
>
>> > Or wait ... are you using compat-wireless?
>>
>> Yes, I am. I mentioned this and did wonder if the _backport/ part
>> in /sys/class is important.
>
> Sorry, didn't see.
>
> Anyway, that's pretty clearly the reason -- Luis added NETDEV_PRE_UP to
> some compat*.h but obviously the kernel won't ever call that notifier,
> so cfg80211 doesn't get a chance to reject the IFUP. No idea how to
> handle that -- it'll be working fine in a regular tree.
>
> Luis, the only way to handle that would be to manually call the PRE_UP
> notifier from mac80211's subif_open() and if that returns an error
> (warning: the calling convention is weird) return the error... that's
> weird but would work.

Okay, that explains it. So I can have a Tested-by: ... I just grep for
NETDEV_PRE_UP in compat-wireless and it is only in
include/net/compat-2.6.31.h (not in 2.6.30) and I am on 2.6.30.5-X . I
can grab the rawhide 2.6.31 rpms and try it quickly;
and possibly look at some ugly quick hack backport that? Stay tuned.

Thanks for the help.

Hin-Tak

2009-08-26 22:13:18

[permalink] [raw]

Subject: Re: [RFC/RFT] rtl8187: Implement rfkill support

On Wed, 2009-08-26 at 15:06 -0700, Luis R. Rodriguez wrote:

> > Neat, thanks. Not sure if I'll get to this this anytime soon, but if
> > someone is interested in it please give it a shot and send a patch.
> > That would be nice.
>
> What if we *also* just do that from mac80211 as well upstream?

Why? It's fine as it is, and we don't want to clutter all cfg80211
drivers with it.

johannes

Attachments:

signature.asc (801.00 B)
This is a digitally signed message part

2009-08-25 01:31:03