Re: [PATCH] pwm: imx27: fix race condition .apply,.get_state

[Leif Middelschulte <leif.middelschulte@gmail.com>]

Hello Uwe,

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

Thank you for supporting the effort.

> 
> On Wed, Jan 25, 2023 at 05:01:42PM +0100, Leif Middelschulte wrote:
>> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
>> 
>> 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.
> 
> As I understand it there is no problem if PWM_DEBUG is off, isn't it?

There is „no problem“ as in: It’s not obvious at the moment/it slumbers. That’s true.

> 
>> 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.
> 
> 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.
> 
> 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.

> 
> 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’ve implemented in v2 of this patch.

> 
> Apart from that, the markdown(?) style you use is unusual for kernel
> commit logs and comments. I'd write:
> 
> With CONFIG_PWM_DEBUG=y 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.

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

Thanks again,

Leif