linux-kernel - Re: [PATCH 1/3] 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: <CAGngYiUELShMgFnvq6XzF0v=2UAwj7gJsPmbdGkmyAbzhM8OLA@mail.gmail.com>
Date:   Thu, 19 Nov 2020 12:29:26 -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 1/3] pwm: pca9685: Switch to atomic API

On Thu, Nov 19, 2020 at 11:00 AM Clemens Gruber
<clemens.gruber@...ruber.com> wrote:
>
> One thing I noticed: The driver currently assumes that it comes out of
> POR in "active" state (comment at end of probe and PM calls).
> However, the SLEEP bit is set by default / after POR.

Very good point, the comment is probably not correct.

It would be wrong to assume that the chip is in any particular
state when probe() runs. There is no reset pin, so the CPU running
Linux could have reset while manipulating the chip, there could even
be leftover state from a bootloader talking to the chip, etc...

I remember running into this myself at the time.

However, we want to make sure that the runtime pm puts the chip to sleep,
no matter its initial state. So the current code is correct, but the
comment can be changed to:

/*
 * Chip activity state unknown. Tell the runtime pm that the chip is
 * active, so pm_runtime_enable() will force it into suspend.
 * Which is what we want as all outputs are disabled at this point.
 */

> 2) If !CONFIG_PM: Clear the SLEEP bit in .probe

Is anyone likely to use this driver without CONFIG_PM? My kernel won't even
build without it...

If you personally have no use for it, then I would not bother with the
!CONFIG_PM case. Just formalize in Kconfig that the driver needs PM.

I think we can add "depends on PM" or "select PM", I am not sure which one
to use here.

Sven