Message-ID: <ZG0itIIg48IGuC8R@orome>
Date:   Tue, 23 May 2023 22:31:48 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Peter Rosin <peda@...ntia.se>, LKML <linux-kernel@...r.kernel.org>,
        linux-pwm@...r.kernel.org,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>
Subject: Re: PWM regression causing failures with the pwm-atmel driver

On Mon, May 22, 2023 at 10:43:46PM +0200, Uwe Kleine-König wrote:
> Hello Peter,
> 
> On Mon, May 22, 2023 at 09:28:39PM +0200, Peter Rosin wrote:
> > 2023-05-22 at 19:23, Uwe Kleine-König wrote:
> > > On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> > >> I have a device with a "sound card" that has an amplifier that needs
> > >> an extra boost when high amplification is requested. This extra
> > >> boost is controlled with a pwm-regulator.
> > >>
> > >> As of commit c73a3107624d ("pwm: Handle .get_state() failures") this
> > >> device no longer works. I have tracked the problem to an unfortunate
> > >> interaction between the underlying PWM driver and the PWM core.
> > >>
> > >> The driver is drivers/pwm/pwm-atmel.c which has difficulties getting
> > >> the period and/or duty_cycle from the HW when the PWM is not enabled.
> > >> Because of this, I think, the driver does not fill in .period and
> > >> .duty_cycle at all in atmel_pwm_get_state() unless the PWM is enabled.
> > >>
> > >> However, the PWM core is not expecting these fields to be left as-is,
> > >> at least not in pwm_adjust_config(), and its local state variable on
> > >> the stack ends up with whatever crap was on the stack on entry for
> > >> these fields. That fails spectacularly when the function continues to
> > >> do math on these uninitialized values.
> 
> After looking again, I don't understand that part. Note that
> pwm_get_state() doesn't call .get_state() at all. Also pwmchip_add()
> zero initializes .state, then pwm_get() calls .get_state() (via
> pwm_device_request() which is called in .xlate()) which (if the HW is
> disabled) doesn't touch .period, so it should continue to be zero?!
> 
> So I wonder why your approach 2 works at all. Do you see what I'm
> missing?
> 
> > >> In particular, I find this in the kernel log when a bad kernel runs:
> > >> pwm-regulator: probe of reg-ana failed with error -22
> > >>
> > >> Before commit c73a3107624d this was a silent failure, and the situation
> > >> "repaired itself" when the PWM was later reprogrammed, at least for my
> > >> case. After that commit, the failure is fatal and the "sound card"
> > >> fails to come up at all.
> > >>
> > >>
> > >> I see a couple of adjustments that could be made.
> > >>
> > >> 1. Zero out some fields in the driver:
> > >>
> > >> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > >>  		state->enabled = true;
> > >>  	} else {
> > >> +		state->period = 0;
> > >> +		state->duty_cycle = 0;
> > >>  		state->enabled = false;
> > >>  	}
> > > 
> > > I don't particularily like that. While state->period is an invalid
> > > value, IMHO enabled = false is enough information from the driver's POV.
> > 
> > This was the preferred approach of Thierry, and given the number of
> > call sites for pwm_get_state with a local variable, I can sympathize
> > with that view.
> 
> I looked a bit more into the issue and think that pwm_get_state() isn't
> problematic. pwm_get_state() fully assigns *state.
> 
> > At the same time, fixing drivers one by one is not
> > a fun game, so I can certainly see that approach 3 also has an appeal.
> 
> What I don't like about it is that the output of a disabled PWM doesn't
> have a period. There might be one configured in hardware(, and the
> .get_state() callback might or might not return that), but the emitted
> signal has no well-defined period.

Well, this is a bit of a gray area, admittedly. .get_state() was not
designed with regards to drivers that are unable to read the hardware
state. Essentially if you can't read the full hardware state, the
assumption was that you just shouldn't provide a .get_state() callback
at all.

In retrospect that's perhaps not ideal, and as you pointed out this is
all a bit moot as of v6.3 because the initial state is now effectively
zeroed out already.

But just to address your point about enabled = false being enough: it's
not. The reason is that consumers should be able to do something along
these lines:

	pwm_get_state(pwm, &state);
	state.enabled = true;
	pwm_apply_state(pwm, &state);

And expect the device to do something reasonable. If the PWM isn't
properly configured, that pwm_apply_state() should return an error. If
->get_state() returned random values, that may not be guaranteed. With
v6.3, the above should return -EINVAL for pwm-atmel because period ends
up being 0 and we check for that explicitly. And that's really the only
sane behavior for drivers that can't read back the full hardware state.

Again, since we have this in the core that should be good enough. But it
is still something that drivers need to be aware about. If you can't
determine a real value from hardware readout, period and duty-cycle
should be zeroed out so that consumers don't end up applying garbage
values accidentally.

Thierry

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

Powered by blists - more mailing lists