Re: [PATCH 03/10] staging: r8188eu: fix status updates in SwLedOff

[Martin Kaiser]

Hi Pavel,

Thus wrote Pavel Skripkin (paskripkin@xxxxxxxxx):

> Hi Martin,

> Martin Kaiser <martin@xxxxxxxxx> says:
> > Update bLedOn only if we could update the REG_LEDCFG2 register.

> > Signed-off-by: Martin Kaiser <martin@xxxxxxxxx>
> > ---
> >   drivers/staging/r8188eu/core/rtw_led.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)

> > diff --git a/drivers/staging/r8188eu/core/rtw_led.c b/drivers/staging/r8188eu/core/rtw_led.c
> > index 4f1cad890cae..38433296d327 100644
> > --- a/drivers/staging/r8188eu/core/rtw_led.c
> > +++ b/drivers/staging/r8188eu/core/rtw_led.c
> > @@ -43,10 +43,11 @@ static void SwLedOn(struct adapter *padapter, struct led_priv *pLed)
> >   static void SwLedOff(struct adapter *padapter, struct led_priv *pLed)
> >   {
> >   	if (padapter->bDriverStopped)
> > -		goto exit;
> > +		return;
> > +
> > +	if (rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3)) != _SUCCESS)
> > +		return;
> > -	rtw_write8(padapter, REG_LEDCFG2, BIT(5) | BIT(3));
> > -exit:
> >   	pLed->bLedOn = false;
> >   }

> If we don't always update the state then, I think, it's better to inform the
> callers about it

> I guess, this won't happen often, but you are changing semantic of the
> function

Changing the state without changing the led feels like a bug to me. It's
done only for SwLedOff, nor for SwLedOn.

We could add a return value and inform the caller that we could not
change the led register.

How would callers of SwLedOn or SwLedLOff handle such errors? blink_work
looks at bLedOn and calls either SwLedOn or SwLedOff. If bLedOn is not
updated and the led is not changed, the next run of the worker will
retry. This does already happen with the current code, a return value of
SwLedOn/Off would not help here.

Best regards,
Martin