[PATCH RFC] b43: stop hardcoding LED behavior

[PATCH RFC] b43: stop hardcoding LED behavior

[Lucas Stach]

The code to hardcode the LED behavior is basically unchanged from when it
was first merged in 2007. It is likely wrong for many modern systems using
the b43 driver.

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. These triggers cause many uneccesary CPU wakeups to drive LEDs
that aren't even present in the system, reducing battery runtime.

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.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
---
 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);
-- 
2.5.5

Re: [PATCH RFC] b43: stop hardcoding LED behavior

[Michael Büsch]

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

On Mon, 25 Apr 2016 09:40:51 +0200
Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org> 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. :)


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


> Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
> ---
>  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);




-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC] b43: stop hardcoding LED behavior

[Lucas Stach]

Am Montag, den 25.04.2016, 17:53 +0200 schrieb Michael Büsch:
> On Mon, 25 Apr 2016 09:40:51 +0200
> Lucas Stach <dev@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 <dev@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);
> 
> 
> 

Re: [PATCH RFC] b43: stop hardcoding LED behavior

[Michael Büsch]

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

On Mon, 25 Apr 2016 20:21:36 +0200
Lucas Stach <dev@lynxeye.de> wrote:

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


Ok this is 40 to 40k for the interrupt requests?
We need real measured numbers and a percentage on how much the b43 LEDs
increase the system wakeups in relation to all other wakeups.


> There are some people that find those kinds of blinking LEDs
> distracting,


Those can already disable them via the standard LED framework.


> so a module parameter to disable them altogether might be
> a good thing to have.


No. We have a standard API for this.


> Causing CPU wakeups in a system where those LEDs
> aren't even physically populated is clearly undesired behavior.


Yes, but this is not going to be fixed via regressions.


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


As it turns out I don't have that card here and I don't have a dump of
its SPROM as I expected. So I cannot really verify this. But I'm pretty
sure that this card did not define any LEDs in its SPROM at all.
I'm not aware of any card that only partially defines LEDs in the
SPROM. So that fix would be OK.

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]