b43 WARN_ON

b43 WARN_ON

[Larry Finger]

Using the latest rfkill patches posted by Johannes, I am hitting the following

        if (WARN_ON(!wl->rfkill.registered))
                return -EINVAL;

in b43_rfkill_soft_set() as follows:

------------[ cut here ]------------
WARNING: at drivers/net/wireless/b43/rfkill.c:77 b43_rfkill_soft_set+0x38/0xb3
[b43]()
Hardware name: HP Pavilion dv2700 Notebook PC
--snip--
---[ end trace a1ef19d1571fdb71 ]---
b43 ssb0:0: firmware: requesting b43/ucode5.fw
b43 ssb0:0: firmware: requesting b43/pcm5.fw
b43 ssb0:0: firmware: requesting b43/b0g0initvals5.fw
b43 ssb0:0: firmware: requesting b43/b0g0bsinitvals5.fw
b43-phy0: Loading firmware version 478.104 (2008-07-01 00:50:23)
b43-phy0 debug: Chip initialized
b43-phy0 debug: 32-bit DMA initialized
Registered led device: b43-phy0::tx
Registered led device: b43-phy0::rx
Registered led device: b43-phy0::radio
b43-phy0 debug: Wireless interface started
b43-phy0 debug: Adding Interface type 2
b43-phy0: Radio turned on by software

Larry

Re: b43 WARN_ON

[Michael Buesch]

On Monday 06 April 2009 05:54:29 Larry Finger wrote:
> Using the latest rfkill patches posted by Johannes, I am hitting the following
> 
>         if (WARN_ON(!wl->rfkill.registered))
>                 return -EINVAL;

I think we should simply remove the warning.
In the previous rfkill implementation I had a reason to _not_ warn here.
(It had one valid codepath where it was called. I don't remember the details...)

Anyway, please feel free to do a patch that removes the warning (but not the check).

> 
> in b43_rfkill_soft_set() as follows:
> 
> ------------[ cut here ]------------
> WARNING: at drivers/net/wireless/b43/rfkill.c:77 b43_rfkill_soft_set+0x38/0xb3
> [b43]()
> Hardware name: HP Pavilion dv2700 Notebook PC
> --snip--
> ---[ end trace a1ef19d1571fdb71 ]---
> b43 ssb0:0: firmware: requesting b43/ucode5.fw
> b43 ssb0:0: firmware: requesting b43/pcm5.fw
> b43 ssb0:0: firmware: requesting b43/b0g0initvals5.fw
> b43 ssb0:0: firmware: requesting b43/b0g0bsinitvals5.fw
> b43-phy0: Loading firmware version 478.104 (2008-07-01 00:50:23)
> b43-phy0 debug: Chip initialized
> b43-phy0 debug: 32-bit DMA initialized
> Registered led device: b43-phy0::tx
> Registered led device: b43-phy0::rx
> Registered led device: b43-phy0::radio
> b43-phy0 debug: Wireless interface started
> b43-phy0 debug: Adding Interface type 2
> b43-phy0: Radio turned on by software
> 
> Larry
> 
> 



-- 
Greetings, Michael.

Re: b43 WARN_ON

[Johannes Berg]

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

On Mon, 2009-04-06 at 12:12 +0200, Michael Buesch wrote:
> On Monday 06 April 2009 05:54:29 Larry Finger wrote:
> > Using the latest rfkill patches posted by Johannes, I am hitting the following
> > 
> >         if (WARN_ON(!wl->rfkill.registered))
> >                 return -EINVAL;
> 
> I think we should simply remove the warning.
> In the previous rfkill implementation I had a reason to _not_ warn here.
> (It had one valid codepath where it was called. I don't remember the details...)
> 
> Anyway, please feel free to do a patch that removes the warning (but not the check).

Actually, I think the check is invalid, or rather, rfkill.registered
needs to be set before registering the rfkill struct.

See, right now, and I'm not entirely happy with this, during
rfkill_register I call the set_block callback. The reason I'm not really
happy with that is that I said calling driver -> rfkill -> driver is
bad, but on the other hand due to locking constraints across the
subsystems the driver cannot take the same locks around rfkill_register
as in the callbacks.

Additionally, the driver must be ready to service rfkill calls _before_
rfkill_register() is called, everything else results in a race
condition. Compare with the shared interrupt handlers, for instance.

The reason I call it during rfkill_register is that I need to sync the
software state to the driver's block state, and that needs to be done
somewhere. Doing that from a work struct would be possible, but wouldn't
solve the locking constraints nor would it actually help since
rfkill_register can potentially sleep, so the work could still be
executed before rfkill_register returns -- the driver still needs to be
able to handle rfkill callbacks before rfkill_register returns.

Therefore, the warning is spurious, but because of a driver bug
elsewhere -- I think the rfkill.registered = 1 assignment should be
moved up to before rfkill_register() (and be = 0 again if that fails
unexpectedly); I'll do that in my patch.

johannes

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

Re: b43 WARN_ON

[Michael Buesch]

On Monday 06 April 2009 12:25:29 Johannes Berg wrote:
> Actually, I think the check is invalid, or rather, rfkill.registered
> needs to be set before registering the rfkill struct.

Would cause a wl->mutex deadlock.

> See, right now, and I'm not entirely happy with this, during
> rfkill_register I call the set_block callback. The reason I'm not really
> happy with that is that I said calling driver -> rfkill -> driver is
> bad, but on the other hand due to locking constraints across the
> subsystems the driver cannot take the same locks around rfkill_register
> as in the callbacks.

Exactly. ;)

> Additionally, the driver must be ready to service rfkill calls _before_
> rfkill_register() is called, everything else results in a race
> condition. Compare with the shared interrupt handlers, for instance.

The driver _is_ ready. It's just not ready to service recursive calls
due to the deadlock. That's what we protect against.

> The reason I call it during rfkill_register is that I need to sync the
> software state to the driver's block state, and that needs to be done
> somewhere. Doing that from a work struct would be possible, but wouldn't
> solve the locking constraints nor would it actually help since
> rfkill_register can potentially sleep, so the work could still be
> executed before rfkill_register returns -- the driver still needs to be
> able to handle rfkill callbacks before rfkill_register returns.

No. The mutex mechanism will make sure there's no deadlock and the scheduler will
schedule the threads in correct order, if you call set_block from the workqueue.
So I think doing the initial set_block from the workqueue asynchronously _would_ solve
the deadlock issue.

> Therefore, the warning is spurious, but because of a driver bug
> elsewhere -- I think the rfkill.registered = 1 assignment should be
> moved up to before rfkill_register() (and be = 0 again if that fails
> unexpectedly); I'll do that in my patch.

As I said. The driver cannot service callbacks while it's registering.
So the registered=1 must remain where it is.

Simply remove the warning, please.

-- 
Greetings, Michael.