linux-kernel - Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

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: <X89RgpTb3sBBI++w@workstation.tuxnet>
Date:   Tue, 8 Dec 2020 11:12:18 +0100
From:   Clemens Gruber <clemens.gruber@...ruber.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     linux-pwm@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Lee Jones <lee.jones@...aro.org>, linux-kernel@...r.kernel.org,
        Sven Van Asbroeck <TheSven73@...il.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        David Jander <david@...tonic.nl>
Subject: Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

Hello Uwe,

On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-König wrote:
> Hello Clemens,
> 
> On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote:
> > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote:
> > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote:
> > > > The hardware readout may return slightly different values than those
> > > > that were set in apply due to the limited range of possible prescale and
> > > > counter register values. If one channel is reconfigured with new duty
> > > > cycle and period, the others will keep the same relative duty cycle to
> > > > period ratio as they had before, even though the per-chip / global
> > > > frequency changed. (The PCA9685 has only one prescaler!)
> > > 
> > > This is not acceptable, if you have two PWM outputs and a consumer
> > > modifies one of them the other must change. So if this chip only
> > > supports a single period length of all channels, the first consumer
> > > enabling a channel defines the period to be used. All later consumers
> > > must live with that. (Also the first must be denied modifying the period
> > > if a second consumer has enabled its PWM.)
> > 
> > Good idea, but is it OK to potentially break users relying on the old
> > behavior ("the last one who changes the period wins") ?
> 
> If this is already in the old code, this probably warrants a separate
> fix, and yes, I consider this a severe bug. (Consider one channel
> driving a motor and reconfiguring an LED modifies the motor's speed.)

Yes, but a user could also be relying on the old behavior as follows:
1. Requests & enables pwm 0 for a backlight, using a period of 5000000ns
   (does not care about the frequency as long as it does not flicker)
2. Requests & enables pwm 1 for a motor, using a period of 1000000ns
   (does care about the frequency)

In the previous kernel versions, this would work, but with your
suggested change, (2) would fail and the motor would no longer work.

We are basically changing "the last one to set the period wins" to "the
first one to set the period wins".

If we do it like this, I'll split it out so we can at least revert it if
someone complains that it breaks his application, without reverting the
whole series.

Thanks,
Clemens