Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\))
Subject: Re: [PATCH] pwm: imx27: fix race condition .apply,.get_state
From:   Leif Middelschulte <leif.middelschulte@gmail.com>
In-Reply-To: <20230125164316.wkoi4qfzbhna3h6g@pengutronix.de>
Date:   Thu, 26 Jan 2023 16:18:45 +0100
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Leif Middelschulte <Leif.Middelschulte@klsmartin.com>,
        linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <76FC6AC2-7588-4C87-9EDF-F7D057683E48@gmail.com>
References: <20230125160142.586358-1-Leif.Middelschulte@gmail.com>
 <20230125164316.wkoi4qfzbhna3h6g@pengutronix.de>
To:     =?utf-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Precedence: bulk

Hello Uwe,

> Am 25.01.2023 um 17:43 schrieb Uwe Kleine-K=C3=B6nig =
<u.kleine-koenig@pengutronix.de>:
>=20
> Hello Leif,
>=20
> first of all thanks for the patch.

Thank you for supporting the effort.

>=20
> On Wed, Jan 25, 2023 at 05:01:42PM +0100, Leif Middelschulte wrote:
>> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
>>=20
>> A race condition might occur, ultimately leading to switching off the
>> PWM that is supposed to be turned on.
>> The condition is more likely, if `CONFIG_PWM_DEBUG` is set and the =
PWM
>> has been enabled before Linux is booted.
>=20
> As I understand it there is no problem if PWM_DEBUG is off, isn't it?

There is =E2=80=9Eno problem=E2=80=9C as in: It=E2=80=99s not obvious at =
the moment/it slumbers. That=E2=80=99s true.

>=20
>> After writing some value to the register linked to the duty cycle
>> (`MX3_PWMSAR`), the related debug function
>> (`core.c:pwm_apply_state_debug`) reads back (`.get_state`)
>> a wrong value (`0`) as the configured duty cycle. This value is =
stored
>> as part of a temporary state variable that it subsequently reapplies
>> to the PWM for testing purposes. Which, effectively, turns off the =
PWM.
>=20
> I thought the thing is: Reading PWMSAR yields the duty_cycle that is
> currently in use. Now if .apply() is called with a new value for =
PWMSAR
> and immediately after that .get_state() reads out PWMSAR the previous
> period (with the previous duty_cycle) probably isn't completed yet and
> so the old value is read.
>=20
> In this case it wouldn't always be 0 which is read. (Hmm, but with the
> conversion we had about this issue, my theory sounds wrong?!)

This is correct. The value will not always be 0, but the current and/or =
future
value of the sample FIFO.

The problem is that it can be quiet hard to tell exactly which value =
will be
read back, as the PWM features a FIFO. The read back value depends on
the timing between register read and write.

>=20
> Maybe instead of waiting in .apply() (which hurts active consumers),
> only wait in .get_state() until MX3_PWMSR_FIFOAV drops to zero?

This is what I=E2=80=99ve implemented in v2 of this patch.

>=20
> Apart from that, the markdown(?) style you use is unusual for kernel
> commit logs and comments. I'd write:
>=20
> With CONFIG_PWM_DEBUG=3Dy after writing a value to the PWMSAR
> register in .apply(), the register is read in .get_state().
> Unless a period completed in the meantime, this read yields the
> previously used duty cycle configuration. As the PWM_DEBUG code
> applies the read out configuration for testing purposes this
> effectively undoes the intended effect by rewriting the previous
> hardware state.

Thank you for pointing this out. I have adopted the suggested =
description
In v2 of this patch.

>=20
> Best regards
> Uwe
>=20
> --=20
> Pengutronix e.K.                           | Uwe Kleine-K=C3=B6nig     =
       |
> Industrial Linux Solutions                 | =
https://www.pengutronix.de/ |

Thanks again,

Leif