linux-kernel - Re: [PATCH v3 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: <CAGngYiViOMO6uM7UeYO5fNMdc+QEjLt+L1TdTii+smTvsmV=aQ@mail.gmail.com>
Date:   Wed, 25 Nov 2020 10:04:32 -0500
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     linux-pwm@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, Lee Jones <lee.jones@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        David Jander <david@...tonic.nl>
Subject: Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API

On Wed, Nov 25, 2020 at 3:35 AM Clemens Gruber
<clemens.gruber@...ruber.com> wrote:
>
> >
> > The datasheet I found for this chip indicates that every ALL_LED_XXX register
> > is write-only. Those registers cannot be read back from the chip.
> >
> > Datasheet "Rev. 4 - 16 April 2015"
>
> Thanks, good catch! Would you agree that we should just return 0 duty
> cycle and disabled state if the "all LEDs" channel is used?

I think get_state() for the all led channel should just return -ENOTSUPP,
if the pwm core will allow that.

Because it's the truth, the chip does not support reading from the all led
channel.

When we start buffering the all led state, we make the code much
more complex, and again we'll run into all sorts of subtle corner cases.

> > > +
> > > +       if (duty < PCA9685_COUNTER_RANGE) {
> >
> > How can duty >= 4096 ?
> >
> > > +               duty *= state->period;
> > > +               state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
> >
> > is this calculation correct?
> > imagine led_on = 0 (default), and led_off = 4095
> > then the led is off for one single tick per cycle
> > but the above formula would calculate it as full on (period == duty_cycle)?

I just wanted to make sure you hadn't overlooked the two comments above.

--

Each time I read the code, my thoughts get interrupted by all this
if hwpwm >= MAXCHAN then { one macro } else { another macro } business
which is spread around in the code !

Same thing with the splitting of the value between H and L registers.
Same thing with the LED_FULL bit.

Maybe the code will be much more readable if we do the following?

- keep pca9685_pwm_full_off/full_on but rename to pca9685_pwm_set_full_off/on
- create pca9685_pwm_is_full_off/on
- create pca9685_pwm_set_on_time/set_off_time

Then LED_FULL, >= MAXCHAN, and register splits are fully confined to
these functions, and we can call them freely in the rest of the code,
without getting confused by these details.

--

> I noticed something else that does not look great:
> Let's say you set up pwm channel 0 with a period of 5000000 and after
> that you set up pwm channel 1 with a period of 40000000.
> So far so good. But if you now set pwm channel 0's period to 5000000
> again, the period stays at 40000000. (Tested with /sys/class/pwm)
>

If the driver isn't buggy, this should work ok. Changing the period on one
channel changes the global prescale, which in turn changes the period on
every other channel. But that's ok, because the ON/OFF times are relative
to a 4096-tick counter, so all duty cycles are preserved.

Example:
1. SET channel 0 : duty  50_000 period 100_000 (real duty cycle = 0.5)
2. SET channel 1 : duty  50_000 period 200_000 (real duty cycle = 0.25)
3. GET channel 0 : duty 100_000 period 200_000 (real duty cycle STILL 0.5)

I think this is acceptable behaviour.

--

I have been thinking about how this patch caches the global prescaler.
There is a possible synchronization issue. Sysfs will always work ok, because
it uses a mutex to protect accesses to pwm_set_state(), which means set_state()
will never be called concurrently.

But I do not think there's any protection when the driver is used as a client
in the devicetree, like so:

&i2c1 {
        my_pca: pwm@0 {
                compatible = "nxp,pca9685-pwm";
                reg = <0>;
        };
};

acme_device@0 {
        pwms = <&my_pca 2 10000000>;
};

acme_device@1 {
        pwms = <&my_pca 1 20000000>;
};

For most pwm drivers, this is fine, because their registers are strictly
separated: writes to channel 0 and 1 do not touch the same registers.

But in our case, because of the cached prescale, things can go very wrong.

I think this can be solved by simply not caching prescale. Everything then
stays on the stack, and the last thread to set the prescaler wins.