linux-kernel - Re: [PATCH] pwm: imx27: fix race condition .apply,.get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 26 Jan 2023 16:18:45 +0100
From:   Leif Middelschulte <leif.middelschulte@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Leif Middelschulte <Leif.Middelschulte@...martin.com>,
        linux-pwm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pwm: imx27: fix race condition .apply,.get_state

Hello Uwe,

> Am 25.01.2023 um 17:43 schrieb Uwe Kleine-König <u.kleine-koenig@...gutronix.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@...martin.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