[PATCH] backlight: ktd253: Stabilize backlight

[PATCH] backlight: ktd253: Stabilize backlight

[Linus Walleij]

Remove interrupt disablement during backlight setting. It is
way to dangerous and makes platforms instable by having it
miss vblank IRQs leading to the graphics derailing.

The code is using ndelay() which is not available on
platforms such as ARM and will result in 32 * udelay(1)
which is substantial.

Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver")
Cc: Stephan Gerhold <stephan@gerhold.net>
Reported-by: newbyte@disroot.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/video/backlight/ktd253-backlight.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/video/backlight/ktd253-backlight.c b/drivers/video/backlight/ktd253-backlight.c
index a7df5bcca9da..b79a47c2801f 100644
--- a/drivers/video/backlight/ktd253-backlight.c
+++ b/drivers/video/backlight/ktd253-backlight.c
@@ -40,7 +40,6 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
 	int brightness = backlight_get_brightness(bl);
 	u16 target_ratio;
 	u16 current_ratio = ktd253->ratio;
-	unsigned long flags;
 
 	dev_dbg(ktd253->dev, "new brightness/ratio: %d/32\n", brightness);
 
@@ -69,13 +68,9 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
 	}
 
 	/*
-	 * WARNING:
-	 * The loop to set the correct current level is performed
-	 * with interrupts disabled as it is timing critical.
 	 * The maximum number of cycles of the loop is 32
 	 * so the time taken will be (T_LOW_NS + T_HIGH_NS + loop_time) * 32,
 	 */
-	local_irq_save(flags);
 	while (current_ratio != target_ratio) {
 		/*
 		 * These GPIO operations absolutely can NOT sleep so no
@@ -92,7 +87,6 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
 		else
 			current_ratio--;
 	}
-	local_irq_restore(flags);
 	ktd253->ratio = current_ratio;
 
 	dev_dbg(ktd253->dev, "new ratio set to %d/32\n", target_ratio);
-- 
2.31.1

Re: [PATCH] backlight: ktd253: Stabilize backlight

[Daniel Thompson]

On Sun, May 30, 2021 at 09:42:38PM +0200, Linus Walleij wrote:
> Remove interrupt disablement during backlight setting. It is
> way to dangerous and makes platforms instable by having it
> miss vblank IRQs leading to the graphics derailing.
> 
> The code is using ndelay() which is not available on
> platforms such as ARM and will result in 32 * udelay(1)
> which is substantial.
> 
> Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver")
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Reported-by: newbyte@disroot.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/video/backlight/ktd253-backlight.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/video/backlight/ktd253-backlight.c b/drivers/video/backlight/ktd253-backlight.c
> index a7df5bcca9da..b79a47c2801f 100644
> --- a/drivers/video/backlight/ktd253-backlight.c
> +++ b/drivers/video/backlight/ktd253-backlight.c
> @@ -40,7 +40,6 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
>  	int brightness = backlight_get_brightness(bl);
>  	u16 target_ratio;
>  	u16 current_ratio = ktd253->ratio;
> -	unsigned long flags;
>  
>  	dev_dbg(ktd253->dev, "new brightness/ratio: %d/32\n", brightness);
>  
> @@ -69,13 +68,9 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
>  	}
>  
>  	/*
> -	 * WARNING:
> -	 * The loop to set the correct current level is performed
> -	 * with interrupts disabled as it is timing critical.
>  	 * The maximum number of cycles of the loop is 32
>  	 * so the time taken will be (T_LOW_NS + T_HIGH_NS + loop_time) * 32,
>  	 */
> -	local_irq_save(flags);
>  	while (current_ratio != target_ratio) {
>  		/*
>  		 * These GPIO operations absolutely can NOT sleep so no
> @@ -92,7 +87,6 @@ static int ktd253_backlight_update_status(struct backlight_device *bl)
>  		else
>  			current_ratio--;
>  	}
> -	local_irq_restore(flags);

IIUC if this code is prevented from progressing for 3ms or longer then
we risk deactivating the backlight instead of setting it. Does there need
to be a retrospective timeout check and recovery loop?


Daniel.


>  	ktd253->ratio = current_ratio;
>  
>  	dev_dbg(ktd253->dev, "new ratio set to %d/32\n", target_ratio);
> -- 
> 2.31.1
> 

Re: [PATCH] backlight: ktd253: Stabilize backlight

[Linus Walleij]

On Tue, Jun 1, 2021 at 10:35 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:

> IIUC if this code is prevented from progressing for 3ms or longer then
> we risk deactivating the backlight instead of setting it. Does there need
> to be a retrospective timeout check and recovery loop?

Hm, could be another patch if we run into it. Newbyte (CC) has tested
it and it works fine and stabilize his system without any problems in
practice with the backlight setting. I would say if users run into this
issue we can look into a timeout-and-restart loop.

You can add:
Tested-by: newbyte@disroot.org
after discussion on IRQ.
As this makes things not crash at least.

Yours,
Linus Walleij