linux-kernel - Re: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support

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, 21 Mar 2019 16:47:08 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Anson Huang <anson.huang@....com>
Cc:     "mark.rutland@....com" <mark.rutland@....com>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        Robin Gong <yibin.gong@....com>,
        "schnitzeltony@...il.com" <schnitzeltony@...il.com>,
        "otavio@...ystems.com.br" <otavio@...ystems.com.br>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "festevam@...il.com" <festevam@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "jan.tuerk@...rion.com" <jan.tuerk@...rion.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "stefan@...er.ch" <stefan@...er.ch>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Leonard Crestez <leonard.crestez@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH V8 2/5] pwm: Add i.MX TPM PWM driver support

Hello,

On Thu, Mar 21, 2019 at 02:53:06PM +0000, Anson Huang wrote:
> > Reading through the reference manual I noticed that there might be a
> > stall: If you write two values to CnV the second write is ignored if the first
> > wasn't latched yet. That might mean that you cannot release the mutex
> > before the newly configured state is active. This is related to the request to
> > not let .apply return before the configured state is active, but I didn't thought
> > this to an end what the real consequences have to be.
> 
> The reference manual says the register is NOT updated until the current period finished
> If counter is running, so I added below check for both period update and duty update, we
> Can just wait the register value read matches what we write:
> 
> Period update:
>          writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
> 
>          /* make sure MOD register is updated */
>          timeout = jiffies + msecs_to_jiffies(tpm->real_period /
>                                               NSEC_PER_MSEC + 1);
>          while (readl(tpm->base + PWM_IMX_TPM_MOD != p->mod)) {
>                  if (time_after(jiffies, timeout))
>                          return = -ETIME;
>                  cpu_relax();
>          }

Hmm, I'm not convinced. There are several things to keep in mind:

 a) you should try to update period and duty_cycle atomically, that is,
    the new values should get both active at the same time. So if the
    PWM is running and the first of the two parameters are written to
    the hardware, the second parameter must be written, too, before the
    current period ends.

 b) A write to CnV or MOD blocks further writes until the respective
    value is "updated" (which I think means "latched into the hardware's
    logic to take effect). So you need to make sure that when updating
    the parameters of a running PWM, you didn't already configure it
    since the current period started.

b) is automatically addressed if you only return from .apply() when the
new configuration is active and hold the mutex during that time.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |