linux-kernel - Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200312084739.isixgdo3txr6rjzg@pengutronix.de>
Date:   Thu, 12 Mar 2020 09:47:39 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Lokesh Vutla <lokeshvutla@...com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Tony Lindgren <tony@...mide.com>,
        Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        Sekhar Nori <nsekhar@...com>, Vignesh R <vigneshr@...com>,
        kernel@...gutronix.de
Subject: Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before
 changing period/duty_cycle

On Thu, Mar 12, 2020 at 01:35:32PM +0530, Lokesh Vutla wrote:
> On 12/03/20 12:10 PM, Uwe Kleine-König wrote:
> > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> >> Only the Timer control register(TCLR) cannot be updated when the timer
> >> is running. Registers like Counter register(TCRR), loader register(TLDR),
> >> match register(TMAR) can be updated when the counter is running. Since
> >> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> >> timer for period/duty_cycle update.
> > 
> > I'm not sure what is sensible here. Stopping the PWM for a short period
> > is bad, but maybe emitting a wrong period isn't better. You can however
> > optimise it if only one of period or duty_cycle changes.
> > 
> > @Thierry, what is your position here? I tend to say a short stop is
> > preferable.
> 
> Short stop has side effects especially in the case where 1PPS is generated using
> this PWM. In this case where PWM period is continuously synced with PTP clock,
> cannot expect any breaks in PWM. This doesn't fall in the above limitations as
> well. as duty_cycle is not a worry and only the rising edge is all that matters.
> 
> Also any specific reason why you wanted to stop rather than having the mentioned
> limitation? it is just a corner anyway and doesn't happen all the time.

I'm a bit torn here. Which of the two steps out of line is worse depends
on what is driven by the PWM in question. And also I think ignoring
"just corner cases" is a reliable way into trouble.

The usual PWM contributer (understandably) cares mostly about their own
problem they have to solve. If however you take a step back and care
about the PWM framework as a whole to be capable to solve problems in
general, such that any consumer just has to know that there is a PWM and
start requesting specific settings for their work to get done, it gets
obvious that you want some kind of uniform behaviour of each hardware
driver. And then a short inactive break between two periods is more
common and better understandable than a mixed period.

Also being a corner case that only happens (say) once in 100000 cases
isn't a clear upside. This just results in a machine leaving the development
department, passing the production test and then behave unexpected once
per year in the field.

Best regards
Uwe

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