linux-kernel - Re: [RFC PATCH] 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: <htgmpafra7fehqfbsusiraed7eny5linma3hrvv3nkuqp3bmuw@zisvojdwymee>
Date: Sun, 16 Jun 2024 09:47:28 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [RFC PATCH] regulator: pwm-regulator: Make assumptions about
 disabled PWM consistent

Hello Martin,

On Sun, Jun 16, 2024 at 01:10:32AM +0200, Martin Blumenstingl wrote:
> On Mon, Jun 10, 2024 at 12:46 PM Uwe Kleine-König
> <u.kleine-koenig@...libre.com> wrote:
> [...]
> > this is my suggestion to fix the concerns I expressed in
> > https://lore.kernel.org/all/hf24mrplr76xtziztkqiscowkh2f3vmceuarecqcwnr6udggs6@uiof2rvvqq5v/
> > .
> >
> > It's only compile tested as I don't have a machine with a pwm-regulator.
> > So getting test feedback before applying it would be great.
> Unfortunately this approach breaks my Odroid-C1 board again with the
> same symptoms as before the mentioned commits: random memory
> corruption, preventing the board from booting to userspace.
> 
> The cause also seems to be the same as before my commits:
> - period (3666ns) and duty cycle (3333ns) in the hardware registers
> the PWM controller when Linux boots, but the PWM output is disabled
> - with a duty cycle of 0 or PWM output being disabled the output of
> the voltage regulator is 1140mV, which is the only allowed voltage for
> that rail (even though the regulator can achieve other voltages)
> - pwm_regulator_enable() calls pwm_enable() which simply takes the
> period and and duty cycle that was read back from the hardware and
> enables the output, resulting in undervolting of a main voltage rail
> of the SoC

Ah, indeed. pwm_enable() looks so innocent, but in fact the details are
difficult. One more reason to drop that caching of parameters in the pwm
core.

> I hope that this (especially the last item) also clarifies the
> question you had in the linked mail on whether updating
> pwm_regulator_init_state() would help/work.
> 
> Regarding your alternative and preferred approach from the other mail:
> > Alternatively (and IMHO nicer) just keep the pwm_state around and don't
> > use the (mis) feature of the PWM core that pwm_get_state only returns
> > the last set state.
> I tried this to see if it would work also for my Odroid-C1 board and
> I'm happy to report it does - see the attached diff.
> In case you are happy with this approach I can submit it as a proper patch.

Yes, I like it. From a quick glance, I only wonder about the dropped
error message in pwm_regulator_set_voltage(). Probably it's right that
this function is silent, but then this is orthogonal to the patch
we're discussing and should go in a separate patch.

Thanks for your valuable cooperation.

Best regards
Uwe

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