* [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom
@ 2006-10-18 4:38 Larry Finger
[not found] ` <4535AFC2.mail3S81JGSDJ-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Larry Finger @ 2006-10-18 4:38 UTC (permalink / raw)
To: John Linville
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w,
Michael Buesch, Stefano Brivio
The bcm43xx driver uses 4 locations in the devices sprom to determine
the behavior of the leds. Certain defaults are assigned if all bits are
set in those locations. On at least one BCM4303 chip, the sprom contains
values other than the default, which executes an assertion placed in the
default case of a following switch statement. This patch makes the leds
on the above mentioned interface behave correctly. In addition, it limits
the number of logged messages to 20 for the case of unexpected values in
the sprom locations.
Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
---
bcm43xx_leds.c | 12 +++++++++++-
bcm43xx_leds.h | 4 ++++
2 files changed, 15 insertions(+), 1 deletion(-)
John,
This patch should be applied to wireless-2.6 so as to be ready for 2.6.20.
Larry
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
@@ -168,6 +168,8 @@ void bcm43xx_leds_exit(struct bcm43xx_pr
bcm43xx_leds_switch_all(bcm, 0);
}
+static int bcm43xx_max_led_err = 20;
+
void bcm43xx_leds_update(struct bcm43xx_private *bcm, int activity)
{
struct bcm43xx_led *led;
@@ -189,20 +191,24 @@ void bcm43xx_leds_update(struct bcm43xx_
case BCM43xx_LED_INACTIVE:
continue;
case BCM43xx_LED_OFF:
+ case BCM43xx_LED_BCM4303_3:
break;
case BCM43xx_LED_ON:
turn_on = 1;
break;
case BCM43xx_LED_ACTIVITY:
+ case BCM43xx_LED_BCM4303_0:
turn_on = activity;
break;
case BCM43xx_LED_RADIO_ALL:
turn_on = radio->enabled;
break;
case BCM43xx_LED_RADIO_A:
+ case BCM43xx_LED_BCM4303_2:
turn_on = (radio->enabled && phy->type == BCM43xx_PHYTYPE_A);
break;
case BCM43xx_LED_RADIO_B:
+ case BCM43xx_LED_BCM4303_1:
turn_on = (radio->enabled &&
(phy->type == BCM43xx_PHYTYPE_B ||
phy->type == BCM43xx_PHYTYPE_G));
@@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
continue;
#endif /* CONFIG_BCM43XX_DEBUG */
default:
- assert(0);
+ if (bcm43xx_max_led_err) {
+ printkl(KERN_INFO PFX "Bad value in leds_update,"
+ " led->behaviour: 0x%x\n", led->behaviour);
+ --bcm43xx_max_led_err;
+ }
};
if (led->activelow)
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
@@ -24,6 +24,10 @@ struct bcm43xx_led {
#define BCM43xx_LED_BEHAVIOUR 0x7F
#define BCM43xx_LED_ACTIVELOW 0x80
+#define BCM43xx_LED_BCM4303_0 0x2B
+#define BCM43xx_LED_BCM4303_1 0x78
+#define BCM43xx_LED_BCM4303_2 0x2E
+#define BCM43xx_LED_BCM4303_3 0x19
enum { /* LED behaviour values */
BCM43xx_LED_OFF,
BCM43xx_LED_ON,
========
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom
[not found] ` <4535AFC2.mail3S81JGSDJ-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
@ 2006-10-18 14:37 ` Michael Buesch
[not found] ` <200610181637.08698.mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Buesch @ 2006-10-18 14:37 UTC (permalink / raw)
To: Larry Finger
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Stefano Brivio, John Linville,
Bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w
On Wednesday 18 October 2006 06:38, Larry Finger wrote:
> The bcm43xx driver uses 4 locations in the devices sprom to determine
> the behavior of the leds. Certain defaults are assigned if all bits are
> set in those locations. On at least one BCM4303 chip, the sprom contains
> values other than the default, which executes an assertion placed in the
> default case of a following switch statement. This patch makes the leds
> on the above mentioned interface behave correctly. In addition, it limits
> the number of logged messages to 20 for the case of unexpected values in
> the sprom locations.
>
> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
>
> bcm43xx_leds.c | 12 +++++++++++-
> bcm43xx_leds.h | 4 ++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
>
> John,
>
> This patch should be applied to wireless-2.6 so as to be ready for 2.6.20.
>
> Larry
>
>
>
>
> Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
> +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.c
> @@ -168,6 +168,8 @@ void bcm43xx_leds_exit(struct bcm43xx_pr
> bcm43xx_leds_switch_all(bcm, 0);
> }
>
> +static int bcm43xx_max_led_err = 20;
> +
> void bcm43xx_leds_update(struct bcm43xx_private *bcm, int activity)
> {
> struct bcm43xx_led *led;
> @@ -189,20 +191,24 @@ void bcm43xx_leds_update(struct bcm43xx_
> case BCM43xx_LED_INACTIVE:
> continue;
> case BCM43xx_LED_OFF:
> + case BCM43xx_LED_BCM4303_3:
> break;
> case BCM43xx_LED_ON:
> turn_on = 1;
> break;
> case BCM43xx_LED_ACTIVITY:
> + case BCM43xx_LED_BCM4303_0:
> turn_on = activity;
> break;
> case BCM43xx_LED_RADIO_ALL:
> turn_on = radio->enabled;
> break;
> case BCM43xx_LED_RADIO_A:
> + case BCM43xx_LED_BCM4303_2:
> turn_on = (radio->enabled && phy->type == BCM43xx_PHYTYPE_A);
> break;
> case BCM43xx_LED_RADIO_B:
> + case BCM43xx_LED_BCM4303_1:
> turn_on = (radio->enabled &&
> (phy->type == BCM43xx_PHYTYPE_B ||
> phy->type == BCM43xx_PHYTYPE_G));
> @@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
> continue;
> #endif /* CONFIG_BCM43XX_DEBUG */
> default:
> - assert(0);
> + if (bcm43xx_max_led_err) {
> + printkl(KERN_INFO PFX "Bad value in leds_update,"
> + " led->behaviour: 0x%x\n", led->behaviour);
> + --bcm43xx_max_led_err;
> + }
I'd call this message bloat. ;) This is the first time the assertion
triggers since it was added.
You could instead remove the assert(), remove bcm43xx_max_led_err
and use dprintkl instead of printkl.
> };
>
> if (led->activelow)
> Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> @@ -24,6 +24,10 @@ struct bcm43xx_led {
>
> #define BCM43xx_LED_BEHAVIOUR 0x7F
> #define BCM43xx_LED_ACTIVELOW 0x80
> +#define BCM43xx_LED_BCM4303_0 0x2B
> +#define BCM43xx_LED_BCM4303_1 0x78
> +#define BCM43xx_LED_BCM4303_2 0x2E
> +#define BCM43xx_LED_BCM4303_3 0x19
Add these to the enum below, instead, please.
The defines above are masks and not the behaviour values.
> enum { /* LED behaviour values */
> BCM43xx_LED_OFF,
> BCM43xx_LED_ON,
>
> ========
>
>
--
Greetings Michael.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom
[not found] ` <200610181637.08698.mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
@ 2006-11-01 3:49 ` John W. Linville
[not found] ` <20061101034936.GD9309-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: John W. Linville @ 2006-11-01 3:49 UTC (permalink / raw)
To: Michael Buesch
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Stefano Brivio,
Bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w, Larry Finger
On Wed, Oct 18, 2006 at 04:37:08PM +0200, Michael Buesch wrote:
> > @@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
> > continue;
> > #endif /* CONFIG_BCM43XX_DEBUG */
> > default:
> > - assert(0);
> > + if (bcm43xx_max_led_err) {
> > + printkl(KERN_INFO PFX "Bad value in leds_update,"
> > + " led->behaviour: 0x%x\n", led->behaviour);
> > + --bcm43xx_max_led_err;
> > + }
>
> I'd call this message bloat. ;) This is the first time the assertion
> triggers since it was added.
> You could instead remove the assert(), remove bcm43xx_max_led_err
> and use dprintkl instead of printkl.
>
> > };
> >
> > if (led->activelow)
> > Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> > ===================================================================
> > --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> > +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_leds.h
> > @@ -24,6 +24,10 @@ struct bcm43xx_led {
> >
> > #define BCM43xx_LED_BEHAVIOUR 0x7F
> > #define BCM43xx_LED_ACTIVELOW 0x80
> > +#define BCM43xx_LED_BCM4303_0 0x2B
> > +#define BCM43xx_LED_BCM4303_1 0x78
> > +#define BCM43xx_LED_BCM4303_2 0x2E
> > +#define BCM43xx_LED_BCM4303_3 0x19
>
> Add these to the enum below, instead, please.
> The defines above are masks and not the behaviour values.
>
> > enum { /* LED behaviour values */
> > BCM43xx_LED_OFF,
> > BCM43xx_LED_ON,
I took Michael's suggestions before merging your patch...FYI! :-)
John
--
John W. Linville
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom
[not found] ` <20061101034936.GD9309-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
@ 2006-11-01 4:34 ` Larry Finger
2006-11-01 11:27 ` Michael Buesch
[not found] ` <454823CC.9070808-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
0 siblings, 2 replies; 6+ messages in thread
From: Larry Finger @ 2006-11-01 4:34 UTC (permalink / raw)
To: John W. Linville
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w,
Michael Buesch, Stefano Brivio
John,
I had not responded to Michael's comments as I heard from another user with thousands of these
assertions in his logs, and I have been waiting for his sprom values and hoped to make a single
patch. It is good, however, that you pushed the patch upstream.
John W. Linville wrote:
> On Wed, Oct 18, 2006 at 04:37:08PM +0200, Michael Buesch wrote:
>
>>> @@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
>>> continue;
>>> #endif /* CONFIG_BCM43XX_DEBUG */
>>> default:
>>> - assert(0);
>>> + if (bcm43xx_max_led_err) {
>>> + printkl(KERN_INFO PFX "Bad value in leds_update,"
>>> + " led->behaviour: 0x%x\n", led->behaviour);
>>> + --bcm43xx_max_led_err;
>>> + }
>> I'd call this message bloat. ;) This is the first time the assertion
>> triggers since it was added.
>> You could instead remove the assert(), remove bcm43xx_max_led_err
>> and use dprintkl instead of printkl.
I disagree with part of Michael's comments. I think we should have a dprintk, rather than dprintkl,
so that we get printouts from all four of the sprom values. That way the user will be able to report
the numbers we need. As this would not limit the log entries and potentially generate thousands,
there should be a variable like bcm43xx_max_led_err to limit the number of log entries.
I will propose a new patch once I get the data for the second case. In the meantime, the patch you
have pushed upstream will fix the BCM4303 led assertions.
Larry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom
2006-11-01 4:34 ` Larry Finger
@ 2006-11-01 11:27 ` Michael Buesch
[not found] ` <454823CC.9070808-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
1 sibling, 0 replies; 6+ messages in thread
From: Michael Buesch @ 2006-11-01 11:27 UTC (permalink / raw)
To: Larry Finger; +Cc: netdev, Bcm43xx-dev, John W. Linville
On Wednesday 01 November 2006 05:34, Larry Finger wrote:
> John,
>
> I had not responded to Michael's comments as I heard from another user with thousands of these
> assertions in his logs, and I have been waiting for his sprom values and hoped to make a single
> patch. It is good, however, that you pushed the patch upstream.
>
> John W. Linville wrote:
> > On Wed, Oct 18, 2006 at 04:37:08PM +0200, Michael Buesch wrote:
> >
> >>> @@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
> >>> continue;
> >>> #endif /* CONFIG_BCM43XX_DEBUG */
> >>> default:
> >>> - assert(0);
> >>> + if (bcm43xx_max_led_err) {
> >>> + printkl(KERN_INFO PFX "Bad value in leds_update,"
> >>> + " led->behaviour: 0x%x\n", led->behaviour);
> >>> + --bcm43xx_max_led_err;
> >>> + }
> >> I'd call this message bloat. ;) This is the first time the assertion
> >> triggers since it was added.
> >> You could instead remove the assert(), remove bcm43xx_max_led_err
> >> and use dprintkl instead of printkl.
>
> I disagree with part of Michael's comments. I think we should have a dprintk, rather than dprintkl,
An unlimited printk will hang the system on UP.
> so that we get printouts from all four of the sprom values.
I don't really think that dprintkl will prevent this.
> That way the user will be able to report
> the numbers we need. As this would not limit the log entries and potentially generate thousands,
> there should be a variable like bcm43xx_max_led_err to limit the number of log entries.
>
> I will propose a new patch once I get the data for the second case. In the meantime, the patch you
> have pushed upstream will fix the BCM4303 led assertions.
I still think it's a waste to add a variable, a printk and a long string which
all eat unswappable kernel memory for this cornercase.
I don't think it's really hard to tell somebody to execute "iwpriv ethX read_sprom"
when he reports the assert() is triggering.
You must communicate with him anyway to find out how the LEDs are mapped
to the physical descriptions on the device case.
--
Greetings Michael.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom
[not found] ` <454823CC.9070808-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
@ 2006-11-01 13:12 ` John W. Linville
0 siblings, 0 replies; 6+ messages in thread
From: John W. Linville @ 2006-11-01 13:12 UTC (permalink / raw)
To: Larry Finger
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w,
Michael Buesch, Stefano Brivio
On Tue, Oct 31, 2006 at 10:34:20PM -0600, Larry Finger wrote:
> John W. Linville wrote:
> >On Wed, Oct 18, 2006 at 04:37:08PM +0200, Michael Buesch wrote:
> >
> >>>@@ -257,7 +263,11 @@ void bcm43xx_leds_update(struct bcm43xx_
> >>> continue;
> >>> #endif /* CONFIG_BCM43XX_DEBUG */
> >>> default:
> >>>- assert(0);
> >>>+ if (bcm43xx_max_led_err) {
> >>>+ printkl(KERN_INFO PFX "Bad value in
> >>>leds_update,"
> >>>+ " led->behaviour: 0x%x\n",
> >>>led->behaviour);
> >>>+ --bcm43xx_max_led_err;
> >>>+ }
> >>I'd call this message bloat. ;) This is the first time the assertion
> >>triggers since it was added.
> >>You could instead remove the assert(), remove bcm43xx_max_led_err
> >>and use dprintkl instead of printkl.
>
> I disagree with part of Michael's comments. I think we should have a
> dprintk, rather than dprintkl, so that we get printouts from all four of
> the sprom values. That way the user will be able to report the numbers we
> need. As this would not limit the log entries and potentially generate
> thousands, there should be a variable like bcm43xx_max_led_err to limit the
> number of log entries.
>
> I will propose a new patch once I get the data for the second case. In the
> meantime, the patch you have pushed upstream will fix the BCM4303 led
> assertions.
OK, cool. I'm happy for you to send another patch.
John
--
John W. Linville
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-11-01 13:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-10-18 4:38 [PATCH] bcm43xx: fix unexpected LED control values in BCM4303 sprom Larry Finger
[not found] ` <4535AFC2.mail3S81JGSDJ-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
2006-10-18 14:37 ` Michael Buesch
[not found] ` <200610181637.08698.mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
2006-11-01 3:49 ` John W. Linville
[not found] ` <20061101034936.GD9309-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
2006-11-01 4:34 ` Larry Finger
2006-11-01 11:27 ` Michael Buesch
[not found] ` <454823CC.9070808-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
2006-11-01 13:12 ` John W. Linville
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).