linux-kernel - Re: pwm-sun4i: PWM backlight is not turned off on shutdown

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: <913a5324-a7d2-f1d5-701e-1c28359286f2@lebedev.lt>
Date:   Wed, 2 Sep 2020 22:05:53 +0300
From:   Andrey Lebedev <andrey@...edev.lt>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Lee Jones <lee.jones@...aro.org>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "open list:PWM SUBSYSTEM" <linux-pwm@...r.kernel.org>,
        "moderated list:ARM/Allwinner sunXi SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Pascal Roeleven <dev@...calroeleven.nl>
Subject: Re: pwm-sun4i: PWM backlight is not turned off on shutdown

On 9/2/20 12:54 PM, Daniel Thompson wrote:
> On Thu, Aug 27, 2020 at 10:55:28PM +0300, Andrey Lebedev wrote:
>> I think I'm experiencing problem with pwm-sun4i module. I'll describe
>> the symptoms first.
>> 
>> I have a device, based on Allwinner A20 (Cubieboard 2) with LVDS display
>> that has a PWM-based backlight. The problem is: when linux shuts down,
>> the backlight stays on. I expect it to be turned off. This used to work
>> as expected on kernel 5.2-rc2, but after upgrade to 5.8 the backlight
>> does not turn off anymore (most of the times, see below).
>> 
>> The backlight is configured in the device tree [1]. The brightness can
>> be changed by writing to "brightness" file on sysfs. So, linux can
>> control the PWM line. Backlight sysfs directory also has a "bl_power"
>> file, which can accept "0" to power on or "4" to power off the backlight
>> (according to [2]).
>> 
>> Now, writing "4" to bl_power sometimes turns the backlight off and
>> sometimes not. I've found that the probability of backlight turning off
>> pretty much correlates with the current screen brightness: on 100%
>> brightness it will never turn off, on 50% brightness it will turn off on
>> about half of the times. When backlight does not turn off, it goes on
>> full brightness. It feels like the line, controlled by pwm stays in
>> whatever state it was the moment backlight was powered down - either
>> full 1 or 0.>>
>> The pwm backlight device driver (pwm_bl) requests to set the duty cycle
>> to 0 and disable the pwm with the same request [3], but I suspect the
>> implementation driver (pwm-sun4i) does not actually set the duty cycle
>> to 0 before disabling the pulse width modulation.
>>
>> Is there anything that can be done to fix this?
> 
> There's some rather odd logic in sun4i_pwm_apply() that results in the
> PWM being disabled twice... once when it applies the initial config
> and again after waiting for a duty_cycle.
> 
> I suspect disabling the initial disable would solve your issue... but it
> might provoke some new ones!
> 
> Anyhow, try removing the else clause starting at line 299 and see what
> happens:
> https://elixir.bootlin.com/linux/v5.8/source/drivers/pwm/pwm-sun4i.c#L299
> 

Thanks Daniel,

Indeed, this fixes the issue for me. The display goes dark reliably on
writing 4 to "/sys/.../bl_power" as well as when system is halted. I did
not notice any negative side effects so far.

I'm not completely sure, but this might be a regression after
d3817a647059a3e5f8791e9b7225d194985aa75f. So, adding Pascal and
Alexandre to the loop.

We'll add this as custom patch to our kernel, will let you know if
something will go wrong because of this.



diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5c677c563349..4b0b9aed9bb9 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -296,9 +296,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
struct pwm_device *pwm,

        if (state->enabled) {
                ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
-       } else {
-               ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);
-               ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
        }

        sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);



-- 
Andrey Lebedev aka -.- . -.. -.. . .-.
Software engineer
Homepage: http://lebedev.lt/