On 10/14/2009 03:06 AM, Michael Buesch wrote:
> On Wednesday 14 October 2009 03:25:30 Larry Finger wrote:
>> Commit 93bad2b757586fb153ef73b028953a8dcaccde77 entitled "b43: Fix PPC crash
>> in rfkill polling on unload" fixed the bug reported in Bugzilla No. 14181;
>> however, it introduced a new bug. Whenever the radio switch was turned off,
>> it was necessary to unload and reload the driver for it to recognize the
>> switch again.
>>
>> I believe this patch fixes the original problem without introducing any new
>> problems.
>>
>> Signed-off-by: Larry Finger <[email protected]>
>> ---
>>
>> John,
>>
>> This is 2.6.32 material to fix the mess I made earlier.
>>
>> Larry
>> ---
>>
>> Index: wireless-testing/drivers/net/wireless/b43/main.c
>> ===================================================================
>> --- wireless-testing.orig/drivers/net/wireless/b43/main.c
>> +++ wireless-testing/drivers/net/wireless/b43/main.c
>> @@ -4501,7 +4501,6 @@ static void b43_op_stop(struct ieee80211
>>
>> cancel_work_sync(&(wl->beacon_update_trigger));
>>
>> - wiphy_rfkill_stop_polling(hw->wiphy);
>> mutex_lock(&wl->mutex);
>> if (b43_status(dev) >= B43_STAT_STARTED) {
>> dev = b43_wireless_core_stop(dev);
>> Index: wireless-testing/drivers/net/wireless/b43/rfkill.c
>> ===================================================================
>> --- wireless-testing.orig/drivers/net/wireless/b43/rfkill.c
>> +++ wireless-testing/drivers/net/wireless/b43/rfkill.c
>> @@ -33,7 +33,8 @@ bool b43_is_hw_radio_enabled(struct b43_
>> & B43_MMIO_RADIO_HWENABLED_HI_MASK))
>> return 1;
>> } else {
>> - if (b43_read16(dev, B43_MMIO_RADIO_HWENABLED_LO)
>> + if (b43_status(dev) >= B43_STAT_STARTED &&
>> + b43_read16(dev, B43_MMIO_RADIO_HWENABLED_LO)
>> & B43_MMIO_RADIO_HWENABLED_LO_MASK)
>> return 1;
>> }
>
> I'm still wondering, however, why you only need that check in the else branch.
> I guess the newer devices which use the other branch just fail silently.
No, the logic in b43_rfkill_poll gets the device into a state that it
works for newer hardware. There is something about the PPC G4 (at
least) that is not in a state where the B43_MMIO_RADIO_HWENABLED_LO
register can be read and the processor faults. I do not have access to
PPC hardware, thus I proposed the above fix as one that would avoid
the fault on PPC, yet not hurt my hardware.
> I don't think this fix is correct at all.
> The only way we can have status<STARTED here is through the rfkill_poll
> function being called before the device is initialized. So it will enable
> the ssb device and try to fetch the rfkill register. _However_, with this
> check
> added it will _always_ return False. So why do we do the polling anyway, if
> it's
> always guaranteed to be false? I think that's buggy.
> To get the same effect, we could just do that:
>
> 44 void b43_rfkill_poll(struct ieee80211_hw *hw)
> 45 {
> 46 struct b43_wl *wl = hw_to_b43_wl(hw);
> 47 struct b43_wldev *dev = wl->current_dev;
> 48 struct ssb_bus *bus = dev->dev->bus;
> 49 bool enabled;
> 50 bool brought_up = false;
> 51
> 52 mutex_lock(&wl->mutex);
> 53 if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
>
> wiphy_rfkill_set_hw_state(false);
> unlock;
> return;
> // Remove the powerup logic...
>
> 54 if (ssb_bus_powerup(bus, 0)) {
> 55 mutex_unlock(&wl->mutex);
> 56 return;
> 57 }
> 58 ssb_device_enable(dev->dev, 0);
> 59 brought_up = true;
> 60 }
>
>
> And no, it's not correct to force device state to STARTED or something else
> here, because
> that won't fix the issue and introduce other bugs.
This code would probably fix the PPC fault, but it would fail for
everything else. Once b43 reports that the RF switch is off, then
mac80211 calls to b43_op_stop, which in turn calls b43_wireless_stop,
and it sets the state to B43_STAT_INITIALIZED. Your proposed code
would have the same failure as my previous patch to stop polling in
b43_op_stop - there would be no way to detect that the switch was back on.
If there is a way to detect that an MMIO read will fail on PPC
architecture, then that is what we need. I do not have a clue on what
that might be.
I'm not sure that any devices that use b43 and have a PHY revision < 3
actually have a radio kill switch. The case in bug 14181 does not.
Would you prefer a patch that assumes that none do, and doesn't start
polling unless phy.rev >= 3? Then we could kill the test in
b43_is_hw_radio_enabled. The routine could even be inlined.
Larry
_______________________________________________
Bcm43xx-dev mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
