Re: PWM regression causing failures with the pwm-atmel driver

[Uwe Kleine-König]

Hello Thierry,

On Tue, May 23, 2023 at 10:31:48PM +0200, Thierry Reding wrote:
> On Mon, May 22, 2023 at 10:43:46PM +0200, Uwe Kleine-König wrote:
> > 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.

This is something that we don't agree about. IMHO it's better if
consumers specify the state they want to apply completely themselves
without using pwm_get_state(). I'm convinced that many people don't
understand pwm_get_state() and the resulting corner cases.

One such corner case is the one we hit here, that pwm_get_state() can
return a state that cannot be reapplied. Another common misunderstanding
is that (apart from directly after driver init (or was it after each
request?)) pwm_get_state() isn't about the actual HW state, but about
the state that was last applied successfully.

Do you think that I should be able to do:

	pwm_apply(mypwm, &(struct pwm_state){ .period = 0, .duty_cycle = 0, .enabled = false, .polarity = PWM_POLARITY_NORMAL });

? This should be enough for a driver to know what to do. After that,
what is the "reasonable" thing that should happen if I follow up with

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

?

The current state is that the first pwm_apply() fails. I didn't check,
but I can imagine that there are other combinations of (state, lowlevel
driver) where pwm_apply() succeeds with .enabled = false but not with
.enabled = true. (For example consider a driver that can only do
inversed polarity, or polarity > 100000. What should happen when I call

	pwm_apply(mypwm, &(struct pwm_state){ .period = 100, .duty_cycle = 20, .enabled = false, .polarity = PWM_POLARITY_NORMAL });

?)

If I was to design a PWM API today, I'd consider to not put .enabled
into pwm_state. With that enabled = true would be implied for
pwm_apply_state(), and there would be a separate pwm_disable().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature