linux-kernel - Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170106192431.GB756@kmp-mobile.hq.kempniu.pl>
Date:   Fri, 6 Jan 2017 20:24:31 +0100
From:   Michał Kępień <kernel@...pniu.pl>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Woithe <jwoithe@...t42.net>,
        Darren Hart <dvhart@...radead.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to
 properly handle errors

> On Thu, Jan 5, 2017 at 3:11 PM, Michał Kępień <kernel@...pniu.pl> wrote:
> > Potential errors returned by some call_fext_func() calls inside
> > logolamp_set() are currently ignored.  Rework logolamp_set() to properly
> > handle them.  This causes one more call_fext_func() call to be made in
> > the LED_OFF case, though one could argue that this is logically the
> > right thing to do (even though the extra call is not needed to shut the
> > LED off).
> >
> > Signed-off-by: Michał Kępień <kernel@...pniu.pl>
> > ---
> > This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
> > use brightness_set_blocking for LED-setting callbacks" and thus needs
> > the latter to be applied first (currently it is already applied in
> > testing).
> >
> > Instead of sticking "if (ret < 0) return ret;" in two branches I decided
> > to restructure logolamp_set() for improved clarity and also to make it
> > resemble logolamp_get() a bit more.
> >
> >  drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index b725a907a91f..12f7a8346dd0 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
> >  static int logolamp_set(struct led_classdev *cdev,
> >                                enum led_brightness brightness)
> >  {
> > -       if (brightness >= LED_FULL) {
> > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
> > -       } else if (brightness >= LED_HALF) {
> > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
> > -       } else {
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);
> 
> > +       int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
> > +
> > +       if (brightness >= LED_HALF) {
> > +               poweron = FUNC_LED_ON;
> > +               if (brightness >= LED_FULL)
> > +                       always = FUNC_LED_ON;
> >         }
> 
> But if you set ON by default the above could be written like
> 
>     if (brightness < LED_FULL)
>         always = FUNC_LED_OFF;
> 
>     if (brightness < LED_HALF)
>         poweron = FUNC_LED_OFF;

Ah, neat, thanks.  Jonathan, assuming you also like this, I will later
submit v2 using this approach.

-- 
Best regards,
Michał Kępień