linux-kernel - Re: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused 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: <3ec5c2bd67cd714f86178a1e7143cd247759aaf8.camel@ew.tq-group.com>
Date:   Tue, 07 Apr 2020 16:46:58 +0200
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Thierry Reding <thierry.reding@...il.com>,
        linux-pwm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Clemens Gruber <clemens.gruber@...ruber.com>
Subject: Re: (EXT) Re: (EXT) Re: [PATCH 1/4] pwm: pca9685: remove unused
 duty_cycle struct element

On Fri, 2020-04-03 at 19:47 -0400, Sven Van Asbroeck wrote:
> On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer
> <matthias.schiffer@...tq-group.com> wrote:
> > 
> > >  - Is this racy somehow (i.e. can it happen that when going from
> > >    duty_cycle/period = 1000/5000 to duty_cycle/period =
> > > 4000/10000
> > > the
> > >    output is 1000/10000 (or 4000/5000) for one cycle)?
> > 
> > It currently is racy. It should be possible to fix that either by
> > updating all 4 registers in a single I2C write, or by using the
> > "update
> > on ACK" mode which requires all 4 registers to be updated before
> > the
> > new setting is applied (I'm not sure if this mode would require
> > using a
> > single I2C write as well though).
> 
> Matthias, did you verify experimentally that changing the period is
> racy?
> 
> Looking at the datasheet and driver code, it shouldn't be. This is
> because
> the OFF time is set as a proportion of the counter range. When the
> period
> changes from 5000 to 10000, then 5000*20%/5000 and 10000*20%/10000
> will result in the same 20% ratio (disregarding rounding errors).
> 
> This is documented at the beginning of the driver:
> 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25
> 
> Should we move that comment to pwm_config(), so future versions of
> ourselves won't overlook it?

You are right, this results in the same ratio - the absolute on/off
times may be wrong for a moment though when the period is changed.

In the attached image, I have changed the period, but kept the absolute
duty cycle fixed (note: this is in inverted mode, so the duty cycle
controls the low time). It can be seen that after a too long high time
(chip is in sleep mode) one period with too long low time follows (new
period, old relative duty cycle), before the counter is reprogrammed to
match the previous absolute duty cycle.

I don't care too much about the details of the behaviour, as I only
control LEDs using this chip and don't need to change the period after
initial setup, but we should accurately document the shortcomings of
the hardware and the driver (when we have decided how to fix some of
the driver issues).

Matthias

Download attachment "osc.jpg" of type "image/jpeg" (51521 bytes)