Thinkpad ACPI led -- it keeps blinking

Hi!

Something is wrong with blinking on thinkpad

pavel@amd:~/bt/blee$ cat /sys/class/leds/tpacpi\:\:power/trigger
[none] bluetooth-power rfkill-any rfkill-none kbd-scrolllock
kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock
kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock
kbd-ctrlrlock AC-online BAT0-charging-or-full BAT0-charging BAT0-full
BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc
phy0radio phy0tpt mmc0 timer heartbeat audio-mute audio-micmute
rfkill1 hci0-power rfkill5
pavel@amd:~/bt/blee$ echo 1 >
/sys/class/leds/tpacpi\:\:power/brightness
pavel@amd:~/bt/blee$ echo 0 >
/sys/class/leds/tpacpi\:\:power/brightness
pavel@amd:~/bt/blee$

Trigger indicates "none" but it keeps blinking, until I echo 0 > brightness to stop it. I have python script to trigger this behaviour... it seems to work from shell.

It behaves strangely. If I ask for blinking twice, it stops blinking on second request.

pavel@amd:~/bt/blee$ echo timer >
/sys/class/leds/tpacpi\:\:power/trigger
pavel@amd:~/bt/blee$ echo timer >
/sys/class/leds/tpacpi\:\:power/trigger

echoing none fixes it.

Kernel 5.1.0-rc1 + some unrelated bits.

Kernel 5.1.0-rc1 + some unrelated bits.

Hi!

Quoted 1 line

Kernel 5.1.0-rc1 + some unrelated bits.

I tried adding
https://marc.info/?l=linux-kernel&m=151622365715313&q=raw as Jacek suggested, and it is still broken.

Test code is this: ledtest actually works as expected on first try, but keeps blinking on second run. Strange.

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

Kernel 5.1.0-rc1 + some unrelated bits.

Quoted 6 lines

I tried adding
https://marc.info/?l=linux-kernel&m=151622365715313&q=raw as Jacek suggested, and it is still broken.

Test code is this: ledtest actually works as expected on first try, but keeps blinking on second run. Strange.

Did it work for previous releases? If yes, then bisect should help here.

Quoted 12 lines · Jacek Anaszewski

Quoted 10 lines · Pavel Machek

Hi!

Quoted 1 line

Kernel 5.1.0-rc1 + some unrelated bits.

I tried adding
https://marc.info/?l=linux-kernel&m=151622365715313&q=raw as Jacek suggested, and it is still broken.

Test code is this: ledtest actually works as expected on first try, but keeps blinking on second run. Strange.

Quoted 1 line

Did it work for previous releases? If yes, then bisect should help here.

Absolutely no idea :-(. I assume "no". Capslock LED on the same system works as expected.

I still hope thinkpad people will speak up, notice it does not work for them, and debug it :-).

Quoted 13 lines · Pavel Machek

Quoted 12 lines · Jacek Anaszewski

Quoted 10 lines · Pavel Machek

Hi!

Quoted 1 line

Kernel 5.1.0-rc1 + some unrelated bits.

I tried adding
https://marc.info/?l=linux-kernel&m=151622365715313&q=raw as Jacek suggested, and it is still broken.

Test code is this: ledtest actually works as expected on first try, but keeps blinking on second run. Strange.

Quoted 7 lines

Quoted 1 line

Did it work for previous releases? If yes, then bisect should help here.

Absolutely no idea :-(. I assume "no". Capslock LED on the same system works as expected.

I still hope thinkpad people will speak up, notice it does not work for them, and debug it :-).

I see this driver implements blink_set:

tpacpi_leds[led].led_classdev.blink_set = &led_sysfs_blink_set;

and also applies some internal logic related to the type of supported LEDs, and the way how the hardware is accessed.

To eliminate the problem on the LED core side you could disable initialization of blink_set op in the driver.

Hi!

This fixes one problem:

Pavel

Signed-off-by Pavel Machek <[email protected]>

drivers/leds/led-core.c +0 -0

index e3da7c0..d795d8f 100644

drivers/leds/led-core.c +1 -0

  	if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
  	    led_cdev->blink_set &&
+	    /* This can sleep */
  	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
  		return;
 

Quoted 9 lines

This fixes one problem:

Signed-off-by Pavel Machek <[email protected]>

drivers/leds/led-core.c +0 -0

index e3da7c0..d795d8f 100644

drivers/leds/led-core.c static void led_blink_setup(struct led_classdev *led_cdev, · line 170

Quoted 7 lines

drivers/leds/led-core.c +4 -0

@@ -164,8 +164,14 @@ static void led_blink_setup(struct led_classdev *led_cdev,
 		     unsigned long *delay_on,
 		     unsigned long *delay_off)
 {
+	while (work_pending(&led_cdev->set_brightness_work)) {
+		printk("Waiting for brightness set to finish\n");
+		schedule();
+	}

Or even better:

flush_work(&led_cdev->set_brightness_work);

Make error returns more consistent... no behaviour change intended.

Signed-off-by Pavel Machek <[email protected]>

drivers/platform/x86/thinkpad_acpi.c +3 -4

index 726341f..7008a7f 100644
@@ -5808,7 +5808,7 @@ static int led_set_status(const unsigned int led,
                        return -EPERM;
                if (!acpi_evalf(led_handle, NULL, NULL, "vdd",
                                (1 << led), led_sled_arg1[ledstatus]))
-                       rc = -EIO;
+                       return -EIO;
                break;
        case TPACPI_LED_OLD:
                /* 600e/x, 770e, 770x, A21e, A2xm/p, T20-22, X20 */

@@ -5832,10 +5832,10 @@ static int led_set_status(const unsigned int led,
                        return -EPERM;
                if (!acpi_evalf(led_handle, NULL, NULL, "vdd",
                                led, led_led_arg1[ledstatus]))
-                       rc = -EIO;
+                       return -EIO;
                break;
        default:
-               rc = -ENXIO;
+               return -ENXIO;
        }
 
        if (!rc)
 
--

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

There are races between "main" thread and workqueue. They manifest themselves on Thinkpad X60:

This should result in LED blinking, but it turns it off instead:

     root@amd:/data/pavel# cd /sys/class/leds/tpacpi\:\:power
     root@amd:/sys/class/leds/tpacpi::power# echo timer > trigger
     root@amd:/sys/class/leds/tpacpi::power# echo timer > trigger
     root@amd:/sys/class/leds/tpacpi::power#

It should be possible to transition from blinking to solid on by echo 0 > brightness; echo 1 > brightness... but that does not work, either, if done too quickly.

Synchronization of the workqueue fixes both.

Signed-off-by Pavel Machek <[email protected]>

drivers/leds/led-class.c +0 -0

index 68aa923..dcb59c8 100644

drivers/leds/led-class.c +0 -0

  	ret = size;
  unlock:

drivers/leds/led-core.c +5 -0

index 9f8da39..aefac4d 100644
@@ -166,6 +166,11 @@ static void led_blink_setup(struct led_classdev *led_cdev,
  		     unsigned long *delay_on,
  		     unsigned long *delay_off)
  {
+	/*
+	 * If "set brightness to 0" is pending in workqueue, we don't
+	 * want that to be reordered after blink_set()
+	 */
+	flush_work(&led_cdev->set_brightness_work);
  	if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
  	    led_cdev->blink_set &&
  	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))

Quoted 13 lines

There are races between "main" thread and workqueue. They manifest themselves on Thinkpad X60:
This should result in LED blinking, but it turns it off instead:

    root@amd:/data/pavel# cd /sys/class/leds/tpacpi\:\:power
    root@amd:/sys/class/leds/tpacpi::power# echo timer > trigger
    root@amd:/sys/class/leds/tpacpi::power# echo timer > trigger
    root@amd:/sys/class/leds/tpacpi::power#
It should be possible to transition from blinking to solid on by echo
0 > brightness; echo 1 > brightness... but that does not work, either,
if done too quickly.
Synchronization of the workqueue fixes both.
Signed-off-by: Pavel Machek <[email protected]>

drivers/leds/led-class.c static ssize_t brightness_store(struct device *dev, · line 60

Quoted 9 lines

drivers/leds/led-class.c +1 -0

index 68aa923..dcb59c8 100644
@@ -57,6 +57,7 @@ static ssize_t brightness_store(struct device *dev,
 	if (state == LED_OFF)
 		led_trigger_remove(led_cdev);
 	led_set_brightness(led_cdev, state);
+	flush_work(&led_cdev->set_brightness_work);

Is this really required here? It creates non-uniform brightness setting behavior depending on whether it is set from sysfs or by in-kernel call to led_set_brightness().

Hi!

Quoted 7 lines

Quoted 7 lines

There are races between "main" thread and workqueue. They manifest themselves on Thinkpad X60:
This should result in LED blinking, but it turns it off instead:

     root@amd:/data/pavel# cd /sys/class/leds/tpacpi\:\:power
     root@amd:/sys/class/leds/tpacpi::power# echo timer > trigger
     root@amd:/sys/class/leds/tpacpi::power# echo timer > trigger
     root@amd:/sys/class/leds/tpacpi::power#

Quoted 6 lines

Quoted 6 lines

It should be possible to transition from blinking to solid on by echo 0 > brightness; echo 1 > brightness... but that does not work, either, if done too quickly.
Synchronization of the workqueue fixes both.

Signed-off-by Pavel Machek <[email protected]>

Quoted 13 lines

drivers/leds/led-class.c static ssize_t brightness_store(struct device *dev, · line 60

Quoted 9 lines

drivers/leds/led-class.c +1 -0

index 68aa923..dcb59c8 100644
@@ -57,6 +57,7 @@ static ssize_t brightness_store(struct device *dev,
  	if (state == LED_OFF)
  		led_trigger_remove(led_cdev);
  	led_set_brightness(led_cdev, state);
+	flush_work(&led_cdev->set_brightness_work);

Is this really required here? It creates non-uniform brightness setting behavior depending on whether it is set from sysfs or by in-kernel call to led_set_brightness().

This fixes the echo 0 > brightness; echo 1 > brightness. It has to be at a place where we can sleep.

If you have better idea, it is welcome, but it would be good to fix the bug.

Quoted 11 lines

Quoted 10 lines

drivers/leds/led-class.c static ssize_t brightness_store(struct device *dev, · line 60

Quoted 6 lines

+++ b/drivers/leds/led-class.c

drivers/leds/led-class.c +1 -0

@@ -57,6 +57,7 @@ static ssize_t brightness_store(struct device *dev,
 	if (state == LED_OFF)
 		led_trigger_remove(led_cdev);
 	led_set_brightness(led_cdev, state);
+	flush_work(&led_cdev->set_brightness_work);

Is this really required here? It creates non-uniform brightness setting behavior depending on whether it is set from sysfs or by in-kernel call to led_set_brightness().

Quoted 5 lines

This fixes the echo 0 > brightness; echo 1 > brightness. It has to be at a place where we can sleep.

If you have better idea, it is welcome, but it would be good to fix the bug.

Currently not, so I applied the patch in this shape.

Hi!

Quoted 17 lines

Quoted 16 lines

Quoted 10 lines

Review static ssize_t brightness_store(struct device *dev, · line 60

Quoted 6 lines

+++ b/drivers/leds/led-class.c

(quoted diff) +1 -0

@@ -57,6 +57,7 @@ static ssize_t brightness_store(struct device *dev,
  	if (state == LED_OFF)
  		led_trigger_remove(led_cdev);
  	led_set_brightness(led_cdev, state);
+	flush_work(&led_cdev->set_brightness_work);

Is this really required here? It creates non-uniform brightness setting behavior depending on whether it is set from sysfs or by in-kernel call to led_set_brightness().

This fixes the echo 0 > brightness; echo 1 > brightness. It has to be at a place where we can sleep.

If you have better idea, it is welcome, but it would be good to fix the bug.

Quoted 1 line

Currently not, so I applied the patch in this shape.

Thanks!

This is actually something that makes sense for stable.. perhaps the bots can pick it up.

Quoted 19 lines · Pavel Machek

Hi!

Quoted 17 lines

Quoted 16 lines

Quoted 10 lines

Review static ssize_t brightness_store(struct device *dev, · line 60

Quoted 6 lines

+++ b/drivers/leds/led-class.c

(quoted diff) +1 -0

@@ -57,6 +57,7 @@ static ssize_t brightness_store(struct device *dev,
 	if (state == LED_OFF)
 		led_trigger_remove(led_cdev);
 	led_set_brightness(led_cdev, state);
+	flush_work(&led_cdev->set_brightness_work);

Is this really required here? It creates non-uniform brightness setting behavior depending on whether it is set from sysfs or by in-kernel call to led_set_brightness().

This fixes the echo 0 > brightness; echo 1 > brightness. It has to be at a place where we can sleep.

If you have better idea, it is welcome, but it would be good to fix the bug.

Quoted 6 lines

Quoted 1 line

Currently not, so I applied the patch in this shape.

Thanks!

This is actually something that makes sense for stable.. perhaps the bots can pick it up.

I was thinking of it, but finally decided to submit this patch to linux-stable when it will prove not having side effects.

But if you think it is ready for stable then I can add relevant "Fixes" tag. Do you think that below will be an appropriate base to refer to?

Fixes 1afcadfcd184 ("leds: core: Use set_brightness_work for the blocking op")

Quoted 34 lines · Jacek Anaszewski

Quoted 19 lines · Pavel Machek

Hi!

Quoted 17 lines

Quoted 16 lines

Quoted 10 lines

Review static ssize_t brightness_store(struct device *dev, · line 60

Quoted 6 lines

+++ b/drivers/leds/led-class.c

(quoted diff) +1 -0

@@ -57,6 +57,7 @@ static ssize_t brightness_store(struct device *dev,
  	if (state == LED_OFF)
  		led_trigger_remove(led_cdev);
  	led_set_brightness(led_cdev, state);
+	flush_work(&led_cdev->set_brightness_work);

Is this really required here? It creates non-uniform brightness setting behavior depending on whether it is set from sysfs or by in-kernel call to led_set_brightness().

This fixes the echo 0 > brightness; echo 1 > brightness. It has to be at a place where we can sleep.

If you have better idea, it is welcome, but it would be good to fix the bug.

Quoted 6 lines

Quoted 1 line

Currently not, so I applied the patch in this shape.

Thanks!

This is actually something that makes sense for stable.. perhaps the bots can pick it up.

I was thinking of it, but finally decided to submit this patch to linux-stable when it will prove not having side effects.

But if you think it is ready for stable then I can add relevant "Fixes" tag. Do you think that below will be an appropriate base to refer to?

Quoted 2 lines

Fixes 1afcadfcd184 ("leds: core: Use set_brightness_work for the blocking op")

Yes, that looks right. If you can add fixes: and cc: stable tags, the rest should happen automagically.

Quoted 2 lines

Yes, that looks right. If you can add fixes: and cc: stable tags, the rest should happen automagically.

Cc: [email protected] is exactly for what it claims - sending a copy to that list.

"Fixes:" seems to be always enough for the bots to pick a patch.

Tag added.