Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 May 2023 18:25:23 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Peter Rosin <peda@axentia.se>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org,
        Thorsten Leemhuis <regressions@leemhuis.info>,
        Conor Dooley <conor.dooley@microchip.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>
Subject: Re: PWM regression causing failures with the pwm-atmel driver
Message-ID: <ZGuXc13mRhqDAVu_@orome>
References: <7e2cfb55-e39f-2e5c-7f43-e1d275428bb5@axentia.se>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="yQ4FAPurPO6iFeq7"
Content-Disposition: inline
In-Reply-To: <7e2cfb55-e39f-2e5c-7f43-e1d275428bb5@axentia.se>
User-Agent: Mutt/2.2.10 (2023-03-25)
Precedence: bulk


--yQ4FAPurPO6iFeq7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, May 22, 2023 at 05:19:43PM +0200, Peter Rosin wrote:
> Hi!
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> In particular, I find this in the kernel log when a bad kernel runs:
> pwm-regulator: probe of reg-ana failed with error -22
>=20
> 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.
>=20
>=20
> I see a couple of adjustments that could be made.
>=20
> 1. Zero out some fields in the driver:
>=20
> @@ -390,4 +390,6 @@ static int atmel_pwm_get_state(struct pwm_chip *chip,=
 struct pwm_device *pwm,
>  		state->enabled =3D true;
>  	} else {
> +		state->period =3D 0;
> +		state->duty_cycle =3D 0;
>  		state->enabled =3D 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

--yQ4FAPurPO6iFeq7
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmRrl3EACgkQ3SOs138+
s6HN6g/9FZdbO0+sT4vNe3BcuLgE8jgBk/xci2tz1QWagmX7V9O8YwAdohK9W6RI
EmqOmjvmShmtUXLzPFvG1TQOSsyCG3OWQpdxxTVOnDIQfg9FawWaLzVmfhhCLrI2
oWsZ7zagEcq9iz76N8BbFBhHK8er1pc82Pg9LNXNDsAGqFZ2N/tl61aFq8SM1DhE
iMh1uk5iP2Z6J0KSQaXBMuBAicyMf1DPsrvR3lXbkI2Z1xVLrwJURDBjYezUk39H
OcRU0vi4wriFkAKhh11UwXl8G+Cft3bRCoCEz1MQM6fXgLP+anpFqTODeK6/18tb
LtbqwsVRhk8mXNOMpAnp0GoMp9Ew5WIJWyzpJv/Qg5QtgjqaBOy3Phnch1+1P2p5
audVNIoKGlWV0EawNVdYVQTWW6koWPsqKttww/tMf8wga5Ojrhu7aZIanGhheBAo
ml0hRxEwmknicjnW5rCnAQW22FzlMNrK936ePBnRoz787YPXLXg5IeFP/d3siPti
9TsoVFF4xl45sg5sxH8c95zUqhQdIQBkHj0mFqbc4D1VNNgRpPlNeVzltH/xBqoV
7OPfPMX3Ft3lIXTBKbzoX7QkroChjeWXRkgDWhN2vSFYDXdNkNYcDZMrtYFyRLZT
70BGoMCl5/f3cQ2hsayHjHbaenbmKrm9MaYcdrw9b7TY8fljF5Q=
=yipu
-----END PGP SIGNATURE-----

--yQ4FAPurPO6iFeq7--