leds: trigger/tty: Use led_set_brightness_nosleep() to set brightness

After commit ba8a86e4dadb ("leds: trigger/tty: Use
led_set_brightness_sync() from workqueue") this is the second try to
pick the right function to set the LED brightness from a trigger.

led_set_brightness_sync() has the problem that it doesn't work for LEDs
without a .brightness_set_blocking() callback. This is (among others)
the case for LEDs connected to non-sleeping GPIOs.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

after a few (non-public and public) reports that the tty trigger doesn't
work and Jacek pointed out in
https://lore.kernel.org/all/ad4a1069-72c6-a431-336f-ed78a57a1ba0@gmail.com/#t
that led_set_brightness_nosleep() is the right function, here comes a
patch to actually implement that.

Does this justify a Fixes line? In that case that would be:

	Fixes: ba8a86e4dadb ("leds: trigger/tty: Use led_set_brightness_sync() from workqueue")

(As ba8a86e4dadb declares to be a fix for fd4a641ac88f ("leds: trigger:
implement a tty trigger") I think a further reference to fd4a641ac88f
isn't necesary.)

Best regards
Uwe

 drivers/leds/trigger/ledtrig-tty.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Hi Uwe,

On 4/14/23 18:48, Uwe Kleine-König wrote:
> After commit ba8a86e4dadb ("leds: trigger/tty: Use
> led_set_brightness_sync() from workqueue") this is the second try to
> pick the right function to set the LED brightness from a trigger.
> 
> led_set_brightness_sync() has the problem that it doesn't work for LEDs
> without a .brightness_set_blocking() callback. This is (among others)
> the case for LEDs connected to non-sleeping GPIOs.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> after a few (non-public and public) reports that the tty trigger doesn't
> work and Jacek pointed out in
> https://lore.kernel.org/all/ad4a1069-72c6-a431-336f-ed78a57a1ba0@gmail.com/#t
> that led_set_brightness_nosleep() is the right function, here comes a
> patch to actually implement that.
> 
> Does this justify a Fixes line? In that case that would be:
> 
> 	Fixes: ba8a86e4dadb ("leds: trigger/tty: Use led_set_brightness_sync() from workqueue")
> 
> (As ba8a86e4dadb declares to be a fix for fd4a641ac88f ("leds: trigger:
> implement a tty trigger") I think a further reference to fd4a641ac88f
> isn't necesary.)
> 
> Best regards
> Uwe
> 
>   drivers/leds/trigger/ledtrig-tty.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index f62db7e520b5..c8bbdeac93b9 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -1,11 +1,11 @@
>   // SPDX-License-Identifier: GPL-2.0
>   
>   #include <linux/delay.h>
> -#include <linux/leds.h>
>   #include <linux/module.h>
>   #include <linux/slab.h>
>   #include <linux/tty.h>
>   #include <uapi/linux/serial.h>
> +#include "../leds.h"
>   
>   struct ledtrig_tty_data {
>   	struct led_classdev *led_cdev;
> @@ -122,12 +122,12 @@ static void ledtrig_tty_work(struct work_struct *work)
>   
>   	if (icount.rx != trigger_data->rx ||
>   	    icount.tx != trigger_data->tx) {
> -		led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> +		led_set_brightness_nosleep(trigger_data->led_cdev, LED_ON);
>   
>   		trigger_data->rx = icount.rx;
>   		trigger_data->tx = icount.tx;
>   	} else {
> -		led_set_brightness_sync(trigger_data->led_cdev, LED_OFF);
> +		led_set_brightness_nosleep(trigger_data->led_cdev, LED_OFF);
>   	}
>   
>   out:
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Hello Uwe,

On 2023-04-14 18:48, Uwe Kleine-König wrote:
> After commit ba8a86e4dadb ("leds: trigger/tty: Use
> led_set_brightness_sync() from workqueue") this is the second try to
> pick the right function to set the LED brightness from a trigger.
> 
> led_set_brightness_sync() has the problem that it doesn't work for LEDs
> without a .brightness_set_blocking() callback. This is (among others)
> the case for LEDs connected to non-sleeping GPIOs.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> after a few (non-public and public) reports that the tty trigger 
> doesn't
> work and Jacek pointed out in
> https://lore.kernel.org/all/ad4a1069-72c6-a431-336f-ed78a57a1ba0@gmail.com/#t
> that led_set_brightness_nosleep() is the right function, here comes a
> patch to actually implement that.
> 
> Does this justify a Fixes line? In that case that would be:
> 
> 	Fixes: ba8a86e4dadb ("leds: trigger/tty: Use
> led_set_brightness_sync() from workqueue")
> 
> (As ba8a86e4dadb declares to be a fix for fd4a641ac88f ("leds: trigger:
> implement a tty trigger") I think a further reference to fd4a641ac88f
> isn't necesary.)
> 
> Best regards
> Uwe
> 
>  drivers/leds/trigger/ledtrig-tty.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c
> b/drivers/leds/trigger/ledtrig-tty.c
> index f62db7e520b5..c8bbdeac93b9 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -1,11 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0
> 
>  #include <linux/delay.h>
> -#include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/tty.h>
>  #include <uapi/linux/serial.h>
> +#include "../leds.h"
> 
>  struct ledtrig_tty_data {
>  	struct led_classdev *led_cdev;
> @@ -122,12 +122,12 @@ static void ledtrig_tty_work(struct work_struct 
> *work)
> 
>  	if (icount.rx != trigger_data->rx ||
>  	    icount.tx != trigger_data->tx) {
> -		led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> +		led_set_brightness_nosleep(trigger_data->led_cdev, LED_ON);
> 
>  		trigger_data->rx = icount.rx;
>  		trigger_data->tx = icount.tx;
>  	} else {
> -		led_set_brightness_sync(trigger_data->led_cdev, LED_OFF);
> +		led_set_brightness_nosleep(trigger_data->led_cdev, LED_OFF);
>  	}
> 
>  out:
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Tested-By: Florian Eckert <fe@dev.tdt.de>

Hi!

> After commit ba8a86e4dadb ("leds: trigger/tty: Use
> led_set_brightness_sync() from workqueue") this is the second try to
> pick the right function to set the LED brightness from a trigger.
> 
> led_set_brightness_sync() has the problem that it doesn't work for LEDs
> without a .brightness_set_blocking() callback. This is (among others)
> the case for LEDs connected to non-sleeping GPIOs.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I don't think this is right.

_nosleep calls _nopm, which assmues it can't sleep, and schedules
another workqueue to set the LED.

Best regards,
									Pavel

Hello,

On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
> > After commit ba8a86e4dadb ("leds: trigger/tty: Use
> > led_set_brightness_sync() from workqueue") this is the second try to
> > pick the right function to set the LED brightness from a trigger.
> > 
> > led_set_brightness_sync() has the problem that it doesn't work for LEDs
> > without a .brightness_set_blocking() callback. This is (among others)
> > the case for LEDs connected to non-sleeping GPIOs.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I don't think this is right.
> 
> _nosleep calls _nopm, which assmues it can't sleep, and schedules
> another workqueue to set the LED.

Then which is the right variant?
led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
(which is bad) and led_set_brightness_sync() doesn't work for some LEDs
(notably LEDs on non-sleeping GPIOs).

From reading the code comments led_set_brightness_sync() sounds like the
right function to call, so maybe we only need to fix gpio-led (and maybe
some others)?

Best regards
Uwe

Hi Uwe,

On 4/17/23 14:44, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
>>> After commit ba8a86e4dadb ("leds: trigger/tty: Use
>>> led_set_brightness_sync() from workqueue") this is the second try to
>>> pick the right function to set the LED brightness from a trigger.
>>>
>>> led_set_brightness_sync() has the problem that it doesn't work for LEDs
>>> without a .brightness_set_blocking() callback. This is (among others)
>>> the case for LEDs connected to non-sleeping GPIOs.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>
>> I don't think this is right.
>>
>> _nosleep calls _nopm, which assmues it can't sleep, and schedules
>> another workqueue to set the LED.
> 
> Then which is the right variant?
> led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
> (which is bad) and led_set_brightness_sync() doesn't work for some LEDs
> (notably LEDs on non-sleeping GPIOs).

Can you remind me the context of this patch, why using workqueue is
bad here?

On Mon, Apr 17, 2023 at 08:33:31PM +0200, Jacek Anaszewski wrote:
> Hi Uwe,
> 
> On 4/17/23 14:44, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
> > > > After commit ba8a86e4dadb ("leds: trigger/tty: Use
> > > > led_set_brightness_sync() from workqueue") this is the second try to
> > > > pick the right function to set the LED brightness from a trigger.
> > > > 
> > > > led_set_brightness_sync() has the problem that it doesn't work for LEDs
> > > > without a .brightness_set_blocking() callback. This is (among others)
> > > > the case for LEDs connected to non-sleeping GPIOs.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > I don't think this is right.
> > > 
> > > _nosleep calls _nopm, which assmues it can't sleep, and schedules
> > > another workqueue to set the LED.
> > 
> > Then which is the right variant?
> > led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
> > (which is bad) and led_set_brightness_sync() doesn't work for some LEDs
> > (notably LEDs on non-sleeping GPIOs).
> 
> Can you remind me the context of this patch, why using workqueue is
> bad here?

The workqueue is only needed if you have a slow LED and want to set the
brightness in atomic context. However if you are allowed to sleep that
is just needless overhead.

Best regards
Uwe

On 4/17/23 21:17, Uwe Kleine-König wrote:
> On Mon, Apr 17, 2023 at 08:33:31PM +0200, Jacek Anaszewski wrote:
>> Hi Uwe,
>>
>> On 4/17/23 14:44, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
>>>>> After commit ba8a86e4dadb ("leds: trigger/tty: Use
>>>>> led_set_brightness_sync() from workqueue") this is the second try to
>>>>> pick the right function to set the LED brightness from a trigger.
>>>>>
>>>>> led_set_brightness_sync() has the problem that it doesn't work for LEDs
>>>>> without a .brightness_set_blocking() callback. This is (among others)
>>>>> the case for LEDs connected to non-sleeping GPIOs.
>>>>>
>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>>
>>>> I don't think this is right.
>>>>
>>>> _nosleep calls _nopm, which assmues it can't sleep, and schedules
>>>> another workqueue to set the LED.
>>>
>>> Then which is the right variant?
>>> led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
>>> (which is bad) and led_set_brightness_sync() doesn't work for some LEDs
>>> (notably LEDs on non-sleeping GPIOs).
>>
>> Can you remind me the context of this patch, why using workqueue is
>> bad here?
> 
> The workqueue is only needed if you have a slow LED and want to set the
> brightness in atomic context. However if you are allowed to sleep that
> is just needless overhead.

OK, now I get it. So new functions will be needed, something like
below (skipped args, need more thinking about corner cases, e.g.
interactions with led_classdev_suspend()):

int led_set_brightness_cansleep()
     led_cdev->brightness = min(value, led_cdev->max_brightness);

     if (led_cdev->flags & LED_SUSPENDED)
         return 0;

     return led_set_brightness_nopm_cansleep();


int led_set_brightness_nopm_cansleep()
     ret =__led_set_brightness();
     if (ret == -ENOTSUPP)
         ret = __led_set_brightness_blocking();

     return ret;

As a quick fix I would apply led_set_brightness_nosleep() nonetheless.
Does it have any observed downsides?

hello Jacek,

On Mon, Apr 17, 2023 at 09:51:06PM +0200, Jacek Anaszewski wrote:
> On 4/17/23 21:17, Uwe Kleine-König wrote:
> > On Mon, Apr 17, 2023 at 08:33:31PM +0200, Jacek Anaszewski wrote:
> > > On 4/17/23 14:44, Uwe Kleine-König wrote:
> > > > On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
> > > > > > After commit ba8a86e4dadb ("leds: trigger/tty: Use
> > > > > > led_set_brightness_sync() from workqueue") this is the second try to
> > > > > > pick the right function to set the LED brightness from a trigger.
> > > > > > 
> > > > > > led_set_brightness_sync() has the problem that it doesn't work for LEDs
> > > > > > without a .brightness_set_blocking() callback. This is (among others)
> > > > > > the case for LEDs connected to non-sleeping GPIOs.
> > > > > > 
> > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > 
> > > > > I don't think this is right.
> > > > > 
> > > > > _nosleep calls _nopm, which assmues it can't sleep, and schedules
> > > > > another workqueue to set the LED.
> > > > 
> > > > Then which is the right variant?
> > > > led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
> > > > (which is bad) and led_set_brightness_sync() doesn't work for some LEDs
> > > > (notably LEDs on non-sleeping GPIOs).
> > > 
> > > Can you remind me the context of this patch, why using workqueue is
> > > bad here?
> > 
> > The workqueue is only needed if you have a slow LED and want to set the
> > brightness in atomic context. However if you are allowed to sleep that
> > is just needless overhead.
> 
> OK, now I get it. So new functions will be needed, something like
> below (skipped args, need more thinking about corner cases, e.g.
> interactions with led_classdev_suspend()):
> 
> int led_set_brightness_cansleep()
>     led_cdev->brightness = min(value, led_cdev->max_brightness);
> 
>     if (led_cdev->flags & LED_SUSPENDED)
>         return 0;
> 
>     return led_set_brightness_nopm_cansleep();
> 
> 
> int led_set_brightness_nopm_cansleep()
>     ret =__led_set_brightness();
>     if (ret == -ENOTSUPP)
>         ret = __led_set_brightness_blocking();
> 
>     return ret;

Did you just reinvent led_set_brightness_sync() and the only thing we
need is:

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index ce4e79939731..4f718609032b 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -83,8 +83,7 @@ static int create_gpio_led(const struct gpio_led *template,
 	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
 	if (!led_dat->can_sleep)
 		led_dat->cdev.brightness_set = gpio_led_set;
-	else
-		led_dat->cdev.brightness_set_blocking = gpio_led_set_blocking;
+	led_dat->cdev.brightness_set_blocking = gpio_led_set_blocking;
 	led_dat->blinking = 0;
 	if (blink_set) {
 		led_dat->platform_gpio_blink_set = blink_set;

?

> As a quick fix I would apply led_set_brightness_nosleep() nonetheless.
> Does it have any observed downsides?

Best regards
Uwe

Hi Uwe,

On 4/18/23 00:27, Uwe Kleine-König wrote:
> hello Jacek,
> 
> On Mon, Apr 17, 2023 at 09:51:06PM +0200, Jacek Anaszewski wrote:
>> On 4/17/23 21:17, Uwe Kleine-König wrote:
>>> On Mon, Apr 17, 2023 at 08:33:31PM +0200, Jacek Anaszewski wrote:
>>>> On 4/17/23 14:44, Uwe Kleine-König wrote:
>>>>> On Mon, Apr 17, 2023 at 02:28:52PM +0200, Pavel Machek wrote:
>>>>>>> After commit ba8a86e4dadb ("leds: trigger/tty: Use
>>>>>>> led_set_brightness_sync() from workqueue") this is the second try to
>>>>>>> pick the right function to set the LED brightness from a trigger.
>>>>>>>
>>>>>>> led_set_brightness_sync() has the problem that it doesn't work for LEDs
>>>>>>> without a .brightness_set_blocking() callback. This is (among others)
>>>>>>> the case for LEDs connected to non-sleeping GPIOs.
>>>>>>>
>>>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>>>>
>>>>>> I don't think this is right.
>>>>>>
>>>>>> _nosleep calls _nopm, which assmues it can't sleep, and schedules
>>>>>> another workqueue to set the LED.
>>>>>
>>>>> Then which is the right variant?
>>>>> led_set_brightness() and led_set_brightness_nosleep() set via a workqueue
>>>>> (which is bad) and led_set_brightness_sync() doesn't work for some LEDs
>>>>> (notably LEDs on non-sleeping GPIOs).
>>>>
>>>> Can you remind me the context of this patch, why using workqueue is
>>>> bad here?
>>>
>>> The workqueue is only needed if you have a slow LED and want to set the
>>> brightness in atomic context. However if you are allowed to sleep that
>>> is just needless overhead.
>>
>> OK, now I get it. So new functions will be needed, something like
>> below (skipped args, need more thinking about corner cases, e.g.
>> interactions with led_classdev_suspend()):
>>
>> int led_set_brightness_cansleep()
>>      led_cdev->brightness = min(value, led_cdev->max_brightness);
>>
>>      if (led_cdev->flags & LED_SUSPENDED)
>>          return 0;
>>
>>      return led_set_brightness_nopm_cansleep();
>>
>>
>> int led_set_brightness_nopm_cansleep()
>>      ret =__led_set_brightness();
>>      if (ret == -ENOTSUPP)
>>          ret = __led_set_brightness_blocking();
>>
>>      return ret;
> 
> Did you just reinvent led_set_brightness_sync() and the only thing we
> need is:

Actually you're right, but led_set_brightness_sync() needs to be
supplemented with the call to __led_set_brightness_blocking().

> 
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index ce4e79939731..4f718609032b 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -83,8 +83,7 @@ static int create_gpio_led(const struct gpio_led *template,
>   	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
>   	if (!led_dat->can_sleep)
>   		led_dat->cdev.brightness_set = gpio_led_set;
> -	else
> -		led_dat->cdev.brightness_set_blocking = gpio_led_set_blocking;
> +	led_dat->cdev.brightness_set_blocking = gpio_led_set_blocking;
>   	led_dat->blinking = 0;
>   	if (blink_set) {
>   		led_dat->platform_gpio_blink_set = blink_set;
> 
> ?
> 
>> As a quick fix I would apply led_set_brightness_nosleep() nonetheless.
>> Does it have any observed downsides?
> 
> Best regards
> Uwe
>

On 4/18/23 20:02, Jacek Anaszewski wrote:
> Hi Uwe,
> 
> On 4/18/23 00:27, Uwe Kleine-König wrote:
>> hello Jacek,
>>
>> On Mon, Apr 17, 2023 at 09:51:06PM +0200, Jacek Anaszewski wrote:
[...]
>>> int led_set_brightness_nopm_cansleep()
>>>      ret =__led_set_brightness();
>>>      if (ret == -ENOTSUPP)
>>>          ret = __led_set_brightness_blocking();
>>>
>>>      return ret;
>>
>> Did you just reinvent led_set_brightness_sync() and the only thing we
>> need is:
> 
> Actually you're right, but led_set_brightness_sync() needs to be
> supplemented with the call to __led_set_brightness_blocking().

I meant __led_set_brightness() of course.