| 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
| ||
|
Date: Tue, 29 Nov 2022 12:25:56 +0000
From: Paul Cercueil <paul@...pouillou.net>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
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
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:
>> Le mar. 25 oct. 2022 à 08:44:10 +0200, Uwe Kleine-König
>> <u.kleine-koenig@...gutronix.de> a écrit :
>> > On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote:
>> > > After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with
>> the
>> > > active part"),
>> > > the trick to set duty > period to properly shut down TCU2
>> channels
>> > > did
>> > > not work anymore, because of the polarity inversion.
>> > >
>> > > Address this issue by restoring the proper polarity before
>> > > disabling the
>> > > channels.
>> > >
>> > > Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the
>> active
>> > > part")
>> > > Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> > > Cc: stable@...r.kernel.org
>> > > ---
>> > > drivers/pwm/pwm-jz4740.c | 62
>> > > ++++++++++++++++++++++++++--------------
>> > > 1 file changed, 40 insertions(+), 22 deletions(-)
>> > >
>> > > diff --git a/drivers/pwm/pwm-jz4740.c
>> b/drivers/pwm/pwm-jz4740.c
>> > > index 228eb104bf1e..65462a0052af 100644
>> > > --- a/drivers/pwm/pwm-jz4740.c
>> > > +++ b/drivers/pwm/pwm-jz4740.c
>> > > @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip
>> > > *chip, struct pwm_device *pwm)
>> > > return 0;
>> > > }
>> > >
>> > > +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip
>> *jz,
>> > > + unsigned int hwpwm,
>> > > + enum pwm_polarity polarity)
>> > > +{
>> > > + unsigned int value = 0;
>> > > +
>> > > + if (polarity == PWM_POLARITY_INVERSED)
>> > > + value = TCU_TCSR_PWM_INITL_HIGH;
>> > > +
>> > > + regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm),
>> > > + TCU_TCSR_PWM_INITL_HIGH, value);
>> > > +}
>> > > +
>> > > static void jz4740_pwm_disable(struct pwm_chip *chip, struct
>> > > pwm_device *pwm)
>> > > {
>> > > struct jz4740_pwm_chip *jz = to_jz4740(chip);
>> > > @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip
>> > > *chip, struct pwm_device *pwm,
>> > > unsigned long long tmp = 0xffffull * NSEC_PER_SEC;
>> > > struct clk *clk = pwm_get_chip_data(pwm);
>> > > unsigned long period, duty;
>> > > + enum pwm_polarity polarity;
>> > > long rate;
>> > > int err;
>> > >
>> > > @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip
>> > > *chip, struct pwm_device *pwm,
>> > > if (duty >= period)
>> > > duty = period - 1;
>> > >
>> > > + /* Restore regular polarity before disabling the channel. */
>> > > + jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity);
>> > > +
>> >
>> > Does this introduce a glitch?
>>
>> Maybe. But the PWM is shut down before finishing its period anyway,
>> so there
>> was already a glitch.
>>
>> > > jz4740_pwm_disable(chip, pwm);
>> > >
>> > > err = clk_set_rate(clk, rate);
>> > > @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct
>> pwm_chip
>> > > *chip, struct pwm_device *pwm,
>> > > regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>> > > TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
>> > >
>> > > - /*
>> > > - * Set polarity.
>> > > - *
>> > > - * The PWM starts in inactive state until the internal timer
>> > > reaches the
>> > > - * duty value, then becomes active until the timer reaches
>> the
>> > > period
>> > > - * value. In theory, we should then use (period - duty) as
>> the
>> > > real duty
>> > > - * value, as a high duty value would otherwise result in the
>> PWM
>> > > pin
>> > > - * being inactive most of the time.
>> > > - *
>> > > - * Here, we don't do that, and instead invert the polarity
>> of the
>> > > PWM
>> > > - * when it is active. This trick makes the PWM start with its
>> > > active
>> > > - * state instead of its inactive state.
>> > > - */
>> > > - if ((state->polarity == PWM_POLARITY_NORMAL) ^
>> state->enabled)
>> > > - regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>> > > - TCU_TCSR_PWM_INITL_HIGH, 0);
>> > > - else
>> > > - regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
>> > > - TCU_TCSR_PWM_INITL_HIGH,
>> > > - TCU_TCSR_PWM_INITL_HIGH);
>> > > -
>> > > - if (state->enabled)
>> > > + if (state->enabled) {
>> > > + /*
>> > > + * Set polarity.
>> > > + *
>> > > + * The PWM starts in inactive state until the internal timer
>> > > + * reaches the duty value, then becomes active until the
>> timer
>> > > + * reaches the period value. In theory, we should then use
>> > > + * (period - duty) as the real duty value, as a high duty
>> value
>> > > + * would otherwise result in the PWM pin being inactive
>> most of
>> > > + * the time.
>> > > + *
>> > > + * Here, we don't do that, and instead invert the polarity
>> of
>> > > + * the PWM when it is active. This trick makes the PWM start
>> > > + * with its active state instead of its inactive state.
>> > > + */
>> > > + if (state->polarity == PWM_POLARITY_NORMAL)
>> > > + polarity = PWM_POLARITY_INVERSED;
>> > > + else
>> > > + polarity = PWM_POLARITY_NORMAL;
>> > > +
>> > > + jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity);
>> > > +
>> > > jz4740_pwm_enable(chip, pwm);
>> > > + }
>> >
>> > 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.
Cheers,
-Paul
Powered by blists - more mailing lists