b43 WARN_ON

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

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).

Greetings, Michael.

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

Would cause a wl->mutex deadlock.

Exactly. ;)

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

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.

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.