linux-kernel - Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

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]

Date:   Tue, 29 Nov 2022 18:46:16 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     Thierry Reding <thierry.reding@...il.com>, od@...ndingux.net,
        linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mips@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2
 channels, part 2

On Tue, Nov 29, 2022 at 04:58:28PM +0000, Paul Cercueil wrote:
> Le mardi 29 novembre 2022 à 17:24 +0100, Uwe Kleine-König a écrit :
> > Hello Paul,
> > 
> > On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> > > Hi Uwe,
> > > 
> > > Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> > > <u.kleine-koenig@...gutronix.de> a écrit :
> > > > Hello,
> > > > 
> > > > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > > > Note that for disabled PWMs there is no official guaranty
> > > > > > about the pin
> > > > > > state. So it would be ok (but admittedly not great) to
> > > > > > simplify the
> > > > > > driver and accept that the pinstate is active while the PWM
> > > > > > is off.
> > > > > > IMHO this is also better than a glitch.
> > > > > > 
> > > > > > If a consumer wants the PWM to be in its inactive state, they
> > > > > > should
> > > > > > not disable it.
> > > > > 
> > > > > Completely disagree. I absolutely do not want the backlight to
> > > > > go full
> > > > > bright mode when the PWM pin is disabled. And disabling the
> > > > > backlight is a
> > > > > thing (for screen blanking and during mode changes).
> > > > 
> > > > For some hardwares there is no pretty choice. So the gist is: If
> > > > the
> > > > backlight driver wants to ensure that the PWM pin is driven to
> > > > its
> > > > inactive level, it should use:
> > > > 
> > > >         pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled
> > > > = true });
> > > > 
> > > > and better not
> > > > 
> > > >         pwm_apply(pwm, { ..., .enabled = false });
> > > 
> > > Well that sounds pretty stupid to me; why doesn't the PWM subsystem
> > > enforce
> > > that the pins must be driven to their inactive level when the PWM
> > > function
> > > is disabled?
> > > 
> > > Then for such hardware you describe, the corresponding PWM
> > > driver could itself apply a duty_cycle = 0 if that's what it takes
> > > to get an
> > > inactive state.
> > 
> > Let's assume we claim that on disable the pin is driven to the
> > inactive level.
> > 
> > The (bad) effect is that for a use case where the pin state doesn't
> > matter (e.g. a backlight where the power regulator is off), the PWM
> > keeps running even though it could be disabled and so save some
> > power.
> > 
> > So to make this use case properly supported, we need another flag in
> > struct pwm_state that allows the consumer to tell the lowlevel driver
> > that it's ok to disable the hardware even with the output being UB.
> > Let's call this new flag "spam" and the pin is allowed to do whatever
> > it
> > wants with .spam = false.
> > 
> > After that you can realize that applying any state with:
> > 
> >         .duty_cycle = A,
> >         .period = B,
> >         .polarity = C,
> >         .enabled = false,
> >         .spam = true,
> > 
> > semantically (i.e. just looking at the output) has the same effect as
> > 
> >         .duty_cycle = 0,
> >         .period = $something,
> >         .polarity = C,
> >         .enabled = true,
> >         .spam = true,
> > 
> > So having .enabled doesn't add to the expressiveness of pwm_apply(),
> > because you can specify any configuration without having to resort to
> > .enabled = false. So the enabled member of struct pwm_state can be
> > dropped.
> > 
> > Then we end up with the exact scenario we have now, just that the
> > flag
> > that specifies if the output should be held in the inactive state has
> > a
> > bad name.
> 
> If I follow you, then it means that the PWM backlight driver pwm_bl.c
> should set state.enabled=true in pwm_backlight_power_off() to make sure
> that the pin is inactive?

Correct, that's the only way to ensure that the pinlevel stays at the
intended level.

And lowlevel PWM drivers can be improved to disable the hardware when
they are asked for .duty_cycle = 0 (maybe under some additional
conditions).

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)