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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> Hi!
> 
> 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.
> 
> 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 prefer this version. Drivers are supposed to set the state as
accurately as they can. If they can't say anything about the hardware
state because it's in an undetermined state, clearing out all the state
fields seems like the best option.

We could probably do this within the core to avoid any such bugs, but it
doesn't really hurt for drivers to be explicit either. Maybe Uwe has
additional thoughts on this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]