linux-kernel - Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 1 Mar 2021 22:52:48 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     Sven Van Asbroeck <thesven73@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-pwm@...r.kernel.org
Subject: Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

Hello,

On Mon, Feb 01, 2021 at 06:24:02PM +0100, Clemens Gruber wrote:
> Hi Sven, Thierry, Uwe,
> 
> On Fri, Jan 29, 2021 at 05:16:51PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> > 
> > On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <thesven73@...il.com> wrote:
> > >
> > > LEN_ON = 409, LED_OFF = 1228 and
> > > LED_ON = 419, LED_OFF = 1238
> > > produce the same result. you can't see the difference between the two
> > > when scoping the channel. there are probably more ways to do this,
> > > some might surprise us. It's a tricky chip.
> > 
> > Please ignore this example, it's bogus. In my defence, it's a Friday
> > afternoon here :)
> 
> Happens to the best of us :)
> 
> > 
> > But consider the following: imagine the bootloader has enabled a few
> > pwm channels, and the driver's .probe() has left them on/unchanged.
> > Then the user enables another pwm channel, and tries to change the
> > period/prescaler. How would pca9685_may_change_prescaler() know
> > if changing the prescaler is allowed?
> > 
> > And the following: imagine the bootloader has enabled a few
> > pwm channels, and the driver's .probe() has left them on/unchanged.
> > After .probe(), the runtime_pm will immediately put the chip to sleep,
> > because it's unaware that some channels are alive.
> 
> (We could read out the state in .probe. If a pwm is already enabled by
> the bootloader, then the user can't change the period. Also, the chip
> would not be put to sleep.
> 
> The user then can export channels and see if they are enabled. If he
> wants to change the period, he needs to find the one enabled by the
> bootloader and change the period there, before he requests more.
> If the bootloader enabled more than one, then he has to disable all but
> one to change the period.
> 
> Or did I miss something?)
> 
> > 
> > I'm sure I'm overlooking a few complications here. probe not changing
> > the existing configuration, will add a lot of complexity to the driver.
> > I'm not saying this is necessarily bad, just a tradeoff. Or, a management
> > decision.
> 
> But I agree that it is simpler if we keep the resets in probe. It would
> also avoid a potentially breaking change for users that do not reset
> their pca9685 chips in their bootloader code.

I would prefer to drop the reset. If the bootloader left with an invalid
state, this is active for sure until the PWM driver is loaded. If you
don't reset, the time is extended (usually) until the consumer comes
along and corrects the setting. So the downside of not resetting is
quite limited, but if you disable the PWM in .probe() the effect can be
worse. And consistency dictates to not reset.

> Removing the resets could then be left as something to discuss further
> in the future and something that belongs in a separate patch series?

That would be fine for me, too.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)