linux-kernel - Re: [RFC PATCH v2] regulator: pwm-regulator: Make assumptions about disabled PWM consistent

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: <y7svv2jvcof3fnezvjbirkian5b7ajic7ajxgtyiejj3364ie7@uvmqlpjj6tv5>
Date: Tue, 6 Aug 2024 08:32:01 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	broonie@...nel.org, lgirdwood@...il.com
Subject: Re: [RFC PATCH v2] regulator: pwm-regulator: Make assumptions about
 disabled PWM consistent

Hello Martin,

On Sun, Jun 23, 2024 at 11:09:08PM +0200, Martin Blumenstingl wrote:
> On Sun, Jun 23, 2024 at 11:32 AM Uwe Kleine-König
> <u.kleine-koenig@...libre.com> wrote:
> > On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> > >       drvdata->state = selector;
> > >
> > > @@ -115,17 +129,26 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
> > >  static int pwm_regulator_enable(struct regulator_dev *dev)
> > >  {
> > >       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > +     struct pwm_state pstate = drvdata->pwm_state;
> > >
> > >       gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
> > >
> > > -     return pwm_enable(drvdata->pwm);
> > > +     pstate.enabled = true;
> > > +
> > > +     return pwm_regulator_apply_state(dev, &pstate);
> > >  }
> > >
> > >  static int pwm_regulator_disable(struct regulator_dev *dev)
> > >  {
> > >       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > +     struct pwm_state pstate = drvdata->pwm_state;
> > > +     int ret;
> > > +
> > > +     pstate.enabled = false;
> > >
> > > -     pwm_disable(drvdata->pwm);
> > > +     ret = pwm_regulator_apply_state(dev, &pstate);
> > > +     if (ret)
> > > +             return ret;
> >
> > With that part I'm a bit unhappy. You don't know what the pwm does when
> > disabled (it might yield a 100% relative duty cycle). So the safe bet is
> > to use .enabled=true, .duty_cycle=0 here.
> >
> > The only code that "knows" if it's safe to disable the PWM is the
> > lowlevel pwm driver.
> Here I don't know the regulator framework enough. Let's make two assumptions:
> 1. the optimization you suggest is implemented (I'm not against it,
> it's just different from what pwm_disable() does)

In general you cannot know how a disabled PWM behaves. The objective of
.enabled = false is to save power and don't care about output. The
typical implementations yield the inactive level, but there are
exceptions that are hard to fix -- if at all. These include High-Z and
the active level. So if the pwm-regulator relies on the PWM emitting the
inactive level, .enabled = false is wrong. I guess in general you don't
know and .enabled = true + .duty_cycle = 0 is the right thing?

> 2. regulator core does not expect the set voltage to change in a
> .disable() callback
> 
> In that case disabling the PWM output is fine. Since we're now
> updating the cached pwm_state with duty_cycle = 0 the next time the
> regulator core calls the .enable() callback (without calling
> .set_voltage() between disabling and enabling) we end up enabling the
> PWM output with duty_cycle = 0 (and thus likely changing the voltage
> output).
> I see three options here:
> - my assumption about the regulator core is incorrect, then your
> optimization works just fine
> - we only write enabled = false to the cached pwm_state but not duty_cycle = 0
> - we drop the suggested optimization here (and maybe let PWM core handle this)
> 
> What do you think?

I'm unsure. I can tell the effect of .enabled = false, but I don't know
if this effect is ok for the pwm-regulator. I also don't know if
disabling and reenabling the regulator is expected to keep the voltage.
Who can tell? I'd hope Mark?

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)