Am Montag, den 25.04.2016, 17:53 +0200 schrieb Michael Büsch:
> On Mon, 25 Apr 2016 09:40:51 +0200
> Lucas Stach <d...@lynxeye.de> wrote:
>
> >
> > On my system the SPROM correctly defines the only wired LED (radio)
> > but
> > skips all others, leading to the hardcode to register LEDs with RX
> > and TX
> > triggers.
> Hm ok. It probably is a good idea to change the condition from
>
> if (sprom[led_index] == 0xFF)
>
> to
>
> if ((sprom[0] & sprom[1] & sprom[2] & sprom[3]) == 0xFF)
>
> So the hardcoding only happens if there is no LED configured in the
> SPROM. (I think my card does this (see below), but I can check that
> later)
>
>
> >
> > These triggers cause many uneccesary CPU wakeups to drive LEDs
> > that aren't even present in the system, reducing battery runtime.
>
> Numbers please. Did you measure that is actually causes more
> _wakeups_?
> How many?
> The led work is placed in the mac80211 workqueue and LED updates only
> happen on behalf of mac80211 activities (by default). It only causes
> additional wakeups, if there's nothing else scheduled on the
> workqueue
> anyways (which might well be the case. So we need numbers. :)
>
The blinking LEDs use a timer to enforce a constant blink rate at a
50ms on/off interval. While they are only triggered if there is some
RX/TX activity in the system, they cause up to 20 wakeups/second/led.
As the timers used for LED activity aren't deferrable, this hardcode is
causing 40 unnecessary CPU wakeups/s in my system.
>
> >
> > Remove the hardcode to stop it from doing any harm. If this code is
> > useful
> > for others it should probably be reworked as a quirk table
> > triggering only
> > for individual systems that need it.
>
> There are cards that need it. I don't know how many that are, but I
> own
> an older 4306 PC-Card card that needs this.
>
> So this effectively is a regression for this card.
>
> So I don't think this is acceptable.
> You should at least make this configurable via module parameter or
> such.
> Or maybe the change from above already is enough. It should work for
> your case.
>
There are some people that find those kinds of blinking LEDs
distracting, so a module parameter to disable them altogether might be
a good thing to have. Causing CPU wakeups in a system where those LEDs
aren't even physically populated is clearly undesired behavior.
If checking that the SPROM doesn't define any LED behavior is enough to
not regress your use case, I would be glad to rework the patch
accordingly.
Regards,
Lucas
>
> >
> > Signed-off-by: Lucas Stach <d...@lynxeye.de>
> > ---
> > drivers/net/wireless/broadcom/b43/leds.c | 26 ++----------------
> > --------
> > 1 file changed, 2 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/b43/leds.c
> > b/drivers/net/wireless/broadcom/b43/leds.c
> > index d79ab2a..77d2dad 100644
> > --- a/drivers/net/wireless/broadcom/b43/leds.c
> > +++ b/drivers/net/wireless/broadcom/b43/leds.c
> > @@ -224,31 +224,9 @@ static void b43_led_get_sprominfo(struct
> > b43_wldev *dev,
> >
> > if (sprom[led_index] == 0xFF) {
> > /* There is no LED information in the SPROM
> > - * for this LED. Hardcode it here. */
> > + * for this LED. Keep it disabled. */
> > *activelow = false;
> > - switch (led_index) {
> > - case 0:
> > - *behaviour = B43_LED_ACTIVITY;
> > - *activelow = true;
> > - if (dev->dev->board_vendor ==
> > PCI_VENDOR_ID_COMPAQ)
> > - *behaviour = B43_LED_RADIO_ALL;
> > - break;
> > - case 1:
> > - *behaviour = B43_LED_RADIO_B;
> > - if (dev->dev->board_vendor ==
> > PCI_VENDOR_ID_ASUSTEK)
> > - *behaviour = B43_LED_ASSOC;
> > - break;
> > - case 2:
> > - *behaviour = B43_LED_RADIO_A;
> > - break;
> > - case 3:
> > - *behaviour = B43_LED_OFF;
> > - break;
> > - default:
> > - *behaviour = B43_LED_OFF;
> > - B43_WARN_ON(1);
> > - return;
> > - }
> > + *behaviour = B43_LED_OFF;
> > } else {
> > *behaviour = sprom[led_index] & B43_LED_BEHAVIOUR;
> > *activelow = !!(sprom[led_index] &
> > B43_LED_ACTIVELOW);
>
>
>
