linux-kernel - Re: [PATCH] input: misc: pwm-vibra: Prevent unbalanced regulator

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAKpie0QcLf+M7YirVcX8AMXQYi8v+_kaKTSC7QZY1-0-op+aSQ@mail.gmail.com>
Date:   Thu, 17 Jan 2019 09:44:51 +0100
From:   Paweł Chmiel <pawel.mikolaj.chmiel@...il.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     linux-input@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jonathan Bakker <xc-racer2@...e.ca>
Subject: Re: [PATCH] input: misc: pwm-vibra: Prevent unbalanced regulator

czw., 17 sty 2019 o 08:08 Dmitry Torokhov <dmitry.torokhov@...il.com>
napisał(a):
>
> Hi Paweł,
>
> On Wed, Jan 16, 2019 at 10:11:31PM +0100, Paweł Chmiel wrote:
> > From: Jonathan Bakker <xc-racer2@...e.ca>
> >
> > pwm_vibrator_stop disables the regulator, but it can be called from
> > multiple places, even when the regulator is already disabled. Fix this
> > by using regulator_is_enabled check when starting and stopping device.
> >
> > Signed-off-by: Jonathan Bakker <xc-racer2@...e.ca>
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@...il.com>
> > ---
> >  drivers/input/misc/pwm-vibra.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> > index 55da191ae550..66677ee770ca 100644
> > --- a/drivers/input/misc/pwm-vibra.c
> > +++ b/drivers/input/misc/pwm-vibra.c
> > @@ -42,10 +42,12 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> >       struct pwm_state state;
> >       int err;
> >
> > -     err = regulator_enable(vibrator->vcc);
> > -     if (err) {
> > -             dev_err(pdev, "failed to enable regulator: %d", err);
> > -             return err;
> > +     if (!regulator_is_enabled(vibrator->vcc)) {
>
> I do not think this is correct in case of shared supply, as this checks
> global state of regulator. That means that if there is another user, we
> may forego enabling regulator here, and that anther user may power it
> down and vibrator will stop working.
>
> I think you need a local flag here.
Ok will fix this (funny that in first version of patch there was such flag).
>
> > +             err = regulator_enable(vibrator->vcc);
> > +             if (err) {
> > +                     dev_err(pdev, "failed to enable regulator: %d", err);
> > +                     return err;
> > +             }
> >       }
> >
> >       pwm_get_state(vibrator->pwm, &state);
> > @@ -76,7 +78,8 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> >
> >  static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
> >  {
> > -     regulator_disable(vibrator->vcc);
> > +     if (regulator_is_enabled(vibrator->vcc))
> > +             regulator_disable(vibrator->vcc);
>
> Looking at this, I wonder if we should not disable PWMs first and the
> disable regulator.
I will create and send separate patch for this.

Thanks for review
>
> >
> >       if (vibrator->pwm_dir)
> >               pwm_disable(vibrator->pwm_dir);
> > --
> > 2.17.1
> >
>
> Thanks.
>
> --
> Dmitry