* staging/rtl8712: unhandled default case in SwLedOn function.
@ 2012-05-02 20:10 joseph daniel
2012-05-02 20:12 ` joseph daniel
0 siblings, 1 reply; 6+ messages in thread
From: joseph daniel @ 2012-05-02 20:10 UTC (permalink / raw)
To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman, Ali Bahar,
linux-kernel, devel
Hi kernel developers,
In the function SwLedOn in rtl8712_led.c, we put the bLedOn = true,
even if its a default case. may be we need to return? or BUG()?.
the code listing is:
if ((padapter->bSurpriseRemoved == true) ||
(padapter->bDriverStopped == true))
return;
LedCfg = r8712_read8(padapter, LEDCFG);
switch (pLed->LedPin) {
case LED_PIN_GPIO0:
break;
case LED_PIN_LED0:
/* SW control led0 on.*/
r8712_write8(padapter, LEDCFG, LedCfg&0xf0);
break;
case LED_PIN_LED1:
/* SW control led1 on.*/
r8712_write8(padapter, LEDCFG, LedCfg&0x0f);
break;
default:
break;
}
pLed->bLedOn = true;
Thanks,
Dev.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: staging/rtl8712: unhandled default case in SwLedOn function. 2012-05-02 20:10 staging/rtl8712: unhandled default case in SwLedOn function joseph daniel @ 2012-05-02 20:12 ` joseph daniel 2012-05-02 20:20 ` Greg Kroah-Hartman 2012-05-02 20:36 ` Larry Finger 0 siblings, 2 replies; 6+ messages in thread From: joseph daniel @ 2012-05-02 20:12 UTC (permalink / raw) To: Larry Finger, Florian Schilhabel, Greg Kroah-Hartman, Ali Bahar, linux-kernel, devel On Thu, May 3, 2012 at 2:10 AM, joseph daniel <josephdanielwalter@gmail.com> wrote: > Hi kernel developers, > > In the function SwLedOn in rtl8712_led.c, we put the bLedOn = true, > even if its a default case. may be we need to return? or BUG()?. > > the code listing is: > > if ((padapter->bSurpriseRemoved == true) || > (padapter->bDriverStopped == true)) > return; > LedCfg = r8712_read8(padapter, LEDCFG); > switch (pLed->LedPin) { > case LED_PIN_GPIO0: > break; > case LED_PIN_LED0: > /* SW control led0 on.*/ > r8712_write8(padapter, LEDCFG, LedCfg&0xf0); > break; > case LED_PIN_LED1: > /* SW control led1 on.*/ > r8712_write8(padapter, LEDCFG, LedCfg&0x0f); > break; > default: /* at this point of the code */ > /* break; */ return; /* or */ /* BUG(); */ /*since we may not be getting into here */ > } > pLed->bLedOn = true; > Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: staging/rtl8712: unhandled default case in SwLedOn function. 2012-05-02 20:12 ` joseph daniel @ 2012-05-02 20:20 ` Greg Kroah-Hartman 2012-05-02 20:36 ` Larry Finger 1 sibling, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2012-05-02 20:20 UTC (permalink / raw) To: joseph daniel Cc: Larry Finger, Florian Schilhabel, Ali Bahar, linux-kernel, devel On Thu, May 03, 2012 at 02:12:39AM +0600, joseph daniel wrote: > On Thu, May 3, 2012 at 2:10 AM, joseph daniel > <josephdanielwalter@gmail.com> wrote: > > Hi kernel developers, > > > > In the function SwLedOn in rtl8712_led.c, we put the bLedOn = true, > > even if its a default case. may be we need to return? or BUG()?. > > > > the code listing is: > > > > if ((padapter->bSurpriseRemoved == true) || > > (padapter->bDriverStopped == true)) > > return; > > LedCfg = r8712_read8(padapter, LEDCFG); > > switch (pLed->LedPin) { > > case LED_PIN_GPIO0: > > break; > > case LED_PIN_LED0: > > /* SW control led0 on.*/ > > r8712_write8(padapter, LEDCFG, LedCfg&0xf0); > > break; > > case LED_PIN_LED1: > > /* SW control led1 on.*/ > > r8712_write8(padapter, LEDCFG, LedCfg&0x0f); > > break; > > default: > /* at this point of the code */ > > /* break; */ > return; /* or */ > /* BUG(); */ /*since we may not be getting into here */ Never crash the kernel in a driver, that's just rude. Error out properly if something unexpected happens. thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: staging/rtl8712: unhandled default case in SwLedOn function. 2012-05-02 20:12 ` joseph daniel 2012-05-02 20:20 ` Greg Kroah-Hartman @ 2012-05-02 20:36 ` Larry Finger 2012-05-03 3:35 ` joseph daniel 2012-05-03 7:26 ` Dan Carpenter 1 sibling, 2 replies; 6+ messages in thread From: Larry Finger @ 2012-05-02 20:36 UTC (permalink / raw) To: joseph daniel Cc: Florian Schilhabel, Greg Kroah-Hartman, Ali Bahar, linux-kernel, devel On 05/02/2012 03:12 PM, joseph daniel wrote: > On Thu, May 3, 2012 at 2:10 AM, joseph daniel > <josephdanielwalter@gmail.com> wrote: >> Hi kernel developers, >> >> In the function SwLedOn in rtl8712_led.c, we put the bLedOn = true, >> even if its a default case. may be we need to return? or BUG()?. >> >> the code listing is: >> >> if ((padapter->bSurpriseRemoved == true) || >> (padapter->bDriverStopped == true)) >> return; >> LedCfg = r8712_read8(padapter, LEDCFG); >> switch (pLed->LedPin) { >> case LED_PIN_GPIO0: >> break; >> case LED_PIN_LED0: >> /* SW control led0 on.*/ >> r8712_write8(padapter, LEDCFG, LedCfg&0xf0); >> break; >> case LED_PIN_LED1: >> /* SW control led1 on.*/ >> r8712_write8(padapter, LEDCFG, LedCfg&0x0f); >> break; >> default: > /* at this point of the code */ >> /* break; */ > return; /* or */ > /* BUG(); */ /*since we may not be getting into here */ >> } >> pLed->bLedOn = true; >> This should do: Index: staging/drivers/staging/rtl8712/rtl8712_led.c =================================================================== --- staging.orig/drivers/staging/rtl8712/rtl8712_led.c +++ staging/drivers/staging/rtl8712/rtl8712_led.c @@ -137,7 +137,8 @@ static void SwLedOn(struct _adapter *pad r8712_write8(padapter, LEDCFG, LedCfg&0x0f); break; default: - break; + WARN_ONCE(1, "Default branch taken in %s\n", __func__); + return; } pLed->bLedOn = true; } If you agree, then I will push the patch to Greg. Larry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: staging/rtl8712: unhandled default case in SwLedOn function. 2012-05-02 20:36 ` Larry Finger @ 2012-05-03 3:35 ` joseph daniel 2012-05-03 7:26 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: joseph daniel @ 2012-05-03 3:35 UTC (permalink / raw) To: Larry Finger Cc: Florian Schilhabel, Greg Kroah-Hartman, Ali Bahar, linux-kernel, devel On Thu, May 3, 2012 at 2:36 AM, Larry Finger <Larry.Finger@lwfinger.net> wrote: > On 05/02/2012 03:12 PM, joseph daniel wrote: >> >> On Thu, May 3, 2012 at 2:10 AM, joseph daniel >> <josephdanielwalter@gmail.com> wrote: >>> >>> Hi kernel developers, >>> >>> In the function SwLedOn in rtl8712_led.c, we put the bLedOn = true, >>> even if its a default case. may be we need to return? or BUG()?. >>> >>> the code listing is: >>> >>> if ((padapter->bSurpriseRemoved == true) || >>> (padapter->bDriverStopped == true)) >>> return; >>> LedCfg = r8712_read8(padapter, LEDCFG); >>> switch (pLed->LedPin) { >>> case LED_PIN_GPIO0: >>> break; >>> case LED_PIN_LED0: >>> /* SW control led0 on.*/ >>> r8712_write8(padapter, LEDCFG, LedCfg&0xf0); >>> break; >>> case LED_PIN_LED1: >>> /* SW control led1 on.*/ >>> r8712_write8(padapter, LEDCFG, LedCfg&0x0f); >>> break; >>> default: >> >> /* at this point of the code */ >>> >>> /* break; */ >> >> return; /* or */ >> /* BUG(); */ /*since we may not be getting into here */ >>> >>> } >>> pLed->bLedOn = true; >>> > > This should do: > > Index: staging/drivers/staging/rtl8712/rtl8712_led.c > =================================================================== > --- staging.orig/drivers/staging/rtl8712/rtl8712_led.c > +++ staging/drivers/staging/rtl8712/rtl8712_led.c > @@ -137,7 +137,8 @@ static void SwLedOn(struct _adapter *pad > > r8712_write8(padapter, LEDCFG, LedCfg&0x0f); > break; > default: > - break; > + WARN_ONCE(1, "Default branch taken in %s\n", __func__); > + return; > } > pLed->bLedOn = true; > } > > If you agree, then I will push the patch to Greg. > Agreed. Thanks Greg and Larry. Acked-by: josephdanielwalter@gmail.com > Larry Thanks, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: staging/rtl8712: unhandled default case in SwLedOn function. 2012-05-02 20:36 ` Larry Finger 2012-05-03 3:35 ` joseph daniel @ 2012-05-03 7:26 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2012-05-03 7:26 UTC (permalink / raw) To: Larry Finger Cc: joseph daniel, devel, Florian Schilhabel, linux-kernel, Greg Kroah-Hartman > This should do: > > Index: staging/drivers/staging/rtl8712/rtl8712_led.c > =================================================================== > --- staging.orig/drivers/staging/rtl8712/rtl8712_led.c > +++ staging/drivers/staging/rtl8712/rtl8712_led.c > @@ -137,7 +137,8 @@ static void SwLedOn(struct _adapter *pad > r8712_write8(padapter, LEDCFG, LedCfg&0x0f); > break; > default: > - break; > + WARN_ONCE(1, "Default branch taken in %s\n", __func__); > + return; > } > pLed->bLedOn = true; > } > Don't just reflexively add extra debug code. In this case pLed->LedPin is either LED_PIN_LED0 or LED_PIN_LED1. The LED_PIN_GPIO0 and default cases are never used. Even if it were I think we would want to set pLed->bLedOn = true in the default case. The code is ugly as pants, but it works fine as is. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-03 7:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-02 20:10 staging/rtl8712: unhandled default case in SwLedOn function joseph daniel 2012-05-02 20:12 ` joseph daniel 2012-05-02 20:20 ` Greg Kroah-Hartman 2012-05-02 20:36 ` Larry Finger 2012-05-03 3:35 ` joseph daniel 2012-05-03 7:26 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox