Thinkpad ACPI led -- it keeps blinking

Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1365 bytes --]

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.

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: Thinkpad ACPI led -- it keeps blinking

[Jacek Anaszewski]

Hi Pavel,

On 4/26/19 2:35 PM, Pavel Machek wrote:
> 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.

Maybe this is manifestation of the possible problem we still
have in the LED core.

I once proposed a patch [0]. You could try if it changes
something in your case.

[0] https://marc.info/?l=linux-kernel&m=151622365715313&w=2

-- 
Best regards,
Jacek Anaszewski

Re: Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 2192 bytes --]

Hi!

> 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.

								Pavel

#!/usr/bin/python
# -*- python -*-
# Copyright Bluez project, GPLv2
# Adapted from test/monitor-bluetooth

from __future__ import absolute_import, print_function, unicode_literals

import sys
import time
import os

import dbus
import dbus.mainloop.glib
try:
  from gi.repository import GObject
except ImportError:
  import gobject as GObject

relevant_ifaces = [ "org.bluez.Device1" ]

class LED:
	def __init__(self):
        	self.path = "/sys/class/leds/tpacpi::power/"
        	#self.path = "/sys/class/leds/input5::capslock/"
                self.brightness = self.path + "brightness"
                self.trigger = self.path + "trigger"

        def set(self, name, val):
        	f = open(name, "w")
                f.write(val)
                f.close()

        def solid(self, b):
        	self.set(self.trigger, "none")
        	self.set(self.brightness, "0")
        	self.set(self.brightness, str(b))

        def blink(self):
        	self.set(self.trigger, "timer")

        def as_root(self):
        	os.chmod(self.trigger, 0777)
        	os.chmod(self.brightness, 0777)

def led_test():
	l = LED()
        l.solid(0)
        time.sleep(2)
	l.solid(1)
        time.sleep(2)
        l.blink()
        time.sleep(2)
	l.solid(1)

def handle_params(args):
        if len(args) < 2 or args[1] == "run":
                LED().blink()
        	run()
                return
        if args[1] == "ledtest":
        	led_test()
                return
        if args[1] == "stop":
                LED().solid(0)
        if args[1] == "startup":
        	LED().as_root()
                return

        print("Unknown parameters", args)

handle_params(sys.argv)




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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: Thinkpad ACPI led -- it keeps blinking

[Jacek Anaszewski]

On 4/26/19 11:42 PM, Pavel Machek wrote:
> Hi!
> 
>> 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.

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

-- 
Best regards,
Jacek Anaszewski

Re: Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 842 bytes --]

On Sat 2019-04-27 18:55:37, Jacek Anaszewski wrote:
> On 4/26/19 11:42 PM, Pavel Machek wrote:
> >Hi!
> >
> >>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.
> 
> 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 :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: Thinkpad ACPI led -- it keeps blinking

[Jacek Anaszewski]

On 4/27/19 9:34 PM, Pavel Machek wrote:
> On Sat 2019-04-27 18:55:37, Jacek Anaszewski wrote:
>> On 4/26/19 11:42 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>> 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.
>>
>> 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.

-- 
Best regards,
Jacek Anaszewski

Re: Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1825 bytes --]

On Sat 2019-04-27 22:35:35, Jacek Anaszewski wrote:
> On 4/27/19 9:34 PM, Pavel Machek wrote:
> >On Sat 2019-04-27 18:55:37, Jacek Anaszewski wrote:
> >>On 4/26/19 11:42 PM, Pavel Machek wrote:
> >>>Hi!
> >>>
> >>>>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.
> >>
> >>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.

Yep, software blinking will very likely work ok.

[  226.949924] LED set 0 to 2
[  226.950766] LED set 0 to 2... 0
[  232.613577] LED set 0 to 2
[  232.613991] LED set 0 to 0
[  232.614467] LED set 0 to 2... 0
[  232.616442] LED set 0 to 0... 0

Thinkpad ACPI driver is being asked to turn led to blink [232.613577]
LED set 0 to 2 and turn it off [  232.613991] LED set 0 to 0
simultaneously. It has no internal locking and ACPI is slow. That
can't end well.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 5017 bytes --]

Hi!

It seems quite clear:

static int timer_trig_activate(struct led_classdev *led_cdev)
{
        printk("timer_trig_activate\n");
	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
	
        led_blink_set(led_cdev, &led_cdev->blink_delay_on, &led_cdev->blink_delay_off);

	printk("timer_trig_activate done\n");

        return 0;
}

static void timer_trig_deactivate(struct led_classdev *led_cdev)
{
        printk("timer_trig_deactivate\n");

	/* Stop blinking */
	led_set_brightness(led_cdev, LED_OFF);

        printk("timer_trig_deactivate done\n");
}

We get timer_trig_deactivate() immediately followed by
timer_trig_activate().

set_brightness goes to workqueue because it would block. That means
that blink_set() happens before set_brightness...

								Pavel

[   16.194141] e1000e 0000:02:00.0 eth1: 10/100 speed: disabling TSO
[  145.887931] timer_trig_activate
[  145.888011] LED set 0 to 2
[  145.888879] LED set 0 to 2... 0
[  145.888893] timer_trig_activate done
[  149.977138] timer_trig_deactivate
[  149.977169] timer_trig_deactivate done
[  149.977479] timer_trig_activate
[  149.977497] LED set 0 to 2
[  149.978281] LED set 0 to 2... 0
[  149.978295] timer_trig_activate done
[  149.978415] LED set 0 to 0
[  149.979851] LED set 0 to 0... 0
[  184.839252] timer_trig_deactivate
[  184.839282] timer_trig_deactivate done
[  184.839319] timer_trig_activate
[  184.839337] LED set 0 to 2
[  184.839907] LED set 0 to 0
[  184.840369] LED set 0 to 2... 0
[  184.840383] timer_trig_activate done
[  184.843318] LED set 0 to 0... 0
root@amd:/sys/class/leds/tpacpi::power#

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index e3da7c0..ca1f69b 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -166,6 +166,7 @@ static void led_blink_setup(struct led_classdev *led_cdev,
 {
 	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;
 
diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index ca898c1..0b061bb5 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -104,6 +104,7 @@ static void pattern_init(struct led_classdev *led_cdev)
 
 static int timer_trig_activate(struct led_classdev *led_cdev)
 {
+	printk("timer_trig_activate\n");
 	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
 		pattern_init(led_cdev);
 		/*
@@ -115,14 +116,18 @@ static int timer_trig_activate(struct led_classdev *led_cdev)
 
 	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
 		      &led_cdev->blink_delay_off);
+	printk("timer_trig_activate done\n");
 
 	return 0;
 }
 
 static void timer_trig_deactivate(struct led_classdev *led_cdev)
 {
+	printk("timer_trig_deactivate\n");
 	/* Stop blinking */
 	led_set_brightness(led_cdev, LED_OFF);
+
+	printk("timer_trig_deactivate done\n");
 }
 
 static struct led_trigger timer_led_trigger = {
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 3a4402a..b3fa9c9 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -363,11 +363,11 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 	int num_ranges, i;
 
 	voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
-	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
 	if (!voltage_ranges) {
 		pr_debug("%pOF: voltage-ranges unspecified\n", np);
 		return 0;
 	}
+	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
 	if (!num_ranges) {
 		pr_err("%pOF: voltage-ranges empty\n", np);
 		return -EINVAL;
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 57d9ae9..3ec6636 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5867,6 +5867,8 @@ static int led_set_status(const unsigned int led,
 
 	int rc = 0;
 
+	printk("LED set %d to %d\n", led, ledstatus);
+
 	switch (led_supported) {
 	case TPACPI_LED_570:
 		/* 570 */
@@ -5876,7 +5878,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 */
@@ -5900,12 +5902,14 @@ 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;
 	}
 
+	printk("LED set %d to %d... %d\n", led, ledstatus, rc);
+
 	if (!rc)
 		tpacpi_led_state_cache[led] = ledstatus;
 

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 912 bytes --]

Hi!

This fixes one problem:

									Pavel

Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index e3da7c0..d795d8f 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -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();
+	}
+		       
 	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;
 

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: Thinkpad ACPI led -- it keeps blinking

[Jacek Anaszewski]

On 4/28/19 12:32 AM, Pavel Machek wrote:
> Hi!
> 
> This fixes one problem:
> 
> 									Pavel
> 
> Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index e3da7c0..d795d8f 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -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);

>   	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;
>   
> 

-- 
Best regards,
Jacek Anaszewski

Re: Thinkpad ACPI led -- it keeps blinking

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 6246 bytes --]

Hi!

> >This fixes one problem:

> >Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> >
> >diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> >index e3da7c0..d795d8f 100644
> >--- a/drivers/leds/led-core.c
> >+++ b/drivers/leds/led-core.c
> >@@ -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);

Yup, thanks for a hint.

I should have acceptable patch, soon; but no promises I catched all
similar bugs, that code is quite ... tricky.

So far I have this:

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..85848c5 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -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);
 
 	ret = size;
 unlock:
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index e3da7c0..e9ae7f8 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -164,6 +164,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))
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 2d451b6..ddfd2dd 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -65,6 +65,7 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
 	up_read(&triggers_list_lock);
 
 unlock:
+	flush_work(&led_cdev->set_brightness_work);
 	mutex_unlock(&led_cdev->led_access);
 	return ret;
 }
diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index ca898c1..0b061bb5 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -104,6 +104,7 @@ static void pattern_init(struct led_classdev *led_cdev)
 
 static int timer_trig_activate(struct led_classdev *led_cdev)
 {
+	printk("timer_trig_activate\n");
 	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
 		pattern_init(led_cdev);
 		/*
@@ -115,14 +116,18 @@ static int timer_trig_activate(struct led_classdev *led_cdev)
 
 	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
 		      &led_cdev->blink_delay_off);
+	printk("timer_trig_activate done\n");
 
 	return 0;
 }
 
 static void timer_trig_deactivate(struct led_classdev *led_cdev)
 {
+	printk("timer_trig_deactivate\n");
 	/* Stop blinking */
 	led_set_brightness(led_cdev, LED_OFF);
+
+	printk("timer_trig_deactivate done\n");
 }
 
 static struct led_trigger timer_led_trigger = {
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 3a4402a..b3fa9c9 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -363,11 +363,11 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 	int num_ranges, i;
 
 	voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
-	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
 	if (!voltage_ranges) {
 		pr_debug("%pOF: voltage-ranges unspecified\n", np);
 		return 0;
 	}
+	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
 	if (!num_ranges) {
 		pr_err("%pOF: voltage-ranges empty\n", np);
 		return -EINVAL;
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 57d9ae9..3580bab 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5867,6 +5867,8 @@ static int led_set_status(const unsigned int led,
 
 	int rc = 0;
 
+	printk("LED set %d to %d\n", led, ledstatus);
+
 	switch (led_supported) {
 	case TPACPI_LED_570:
 		/* 570 */
@@ -5876,7 +5878,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 */
@@ -5900,12 +5902,14 @@ 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;
 	}
 
+	printk("LED set %d to %d... %d\n", led, ledstatus, rc);
+
 	if (!rc)
 		tpacpi_led_state_cache[led] = ledstatus;
 
@@ -5919,6 +5923,8 @@ static int led_sysfs_set(struct led_classdev *led_cdev,
 			     struct tpacpi_led_classdev, led_classdev);
 	enum led_status_t new_state;
 
+	printk("sysfs_set %d\n", brightness);
+
 	if (brightness == LED_OFF)
 		new_state = TPACPI_LED_OFF;
 	else if (tpacpi_led_state_cache[data->led] != TPACPI_LED_BLINK)
@@ -5935,6 +5941,8 @@ static int led_sysfs_blink_set(struct led_classdev *led_cdev,
 	struct tpacpi_led_classdev *data = container_of(led_cdev,
 			     struct tpacpi_led_classdev, led_classdev);
 
+	printk("sysfs_blink_set\n");
+
 	/* Can we choose the flash rate? */
 	if (*delay_on == 0 && *delay_off == 0) {
 		/* yes. set them to the hardware blink rate (1 Hz) */
@@ -5943,6 +5951,7 @@ static int led_sysfs_blink_set(struct led_classdev *led_cdev,
 	} else if ((*delay_on != 500) || (*delay_off != 500))
 		return -EINVAL;
 
+	printk("sysfs_blink_set: hardware can do it\n");
 	return led_set_status(data->led, TPACPI_LED_BLINK);
 }
 

								Pavel

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

[PATCH] leds: tpacpi: cleanup for Thinkpad ACPI led

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1105 bytes --]


Make error returns more consistent... no behaviour change intended.
    
Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 726341f..7008a7f 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH] leds: tpacpi: cleanup for Thinkpad ACPI led

[Andy Shevchenko]

On Mon, Apr 29, 2019 at 6:22 PM Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> wrote:
>
>
> Make error returns more consistent... no behaviour change intended.
>

Pushed to my review and testing queue, thanks!

P.S. I fixed prefix accordingly.

> Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 726341f..7008a7f 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -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



-- 
With Best Regards,
Andy Shevchenko

[PATCH] leds: avoid races with workqueue

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1898 bytes --]


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 <pavel-+ZI9xUNit7I@public.gmane.org>

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 68aa923..dcb59c8 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -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);
 
 	ret = size;
 unlock:
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 9f8da39..aefac4d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -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))

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH] leds: avoid races with workqueue

[Jacek Anaszewski]

Hi Pavel,

Thank you for the patch.

On 4/29/19 5:22 PM, Pavel Machek wrote:
> 
> 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 <pavel-+ZI9xUNit7I@public.gmane.org>
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 68aa923..dcb59c8 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -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().

>   	ret = size;
>   unlock:
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 9f8da39..aefac4d 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -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))
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: avoid races with workqueue

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1688 bytes --]

Hi!

> >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 <pavel-+ZI9xUNit7I@public.gmane.org>
> >
> >diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> >index 68aa923..dcb59c8 100644
> >--- a/drivers/leds/led-class.c
> >+++ b/drivers/leds/led-class.c
> >@@ -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.

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH] leds: avoid races with workqueue

[Jacek Anaszewski]

Hi Pavel,

On 5/1/19 8:36 PM, Pavel Machek wrote:
> Hi!
> 
>>> 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#

I believe this line is redundant, so I removed it.

>>> 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 <pavel-+ZI9xUNit7I@public.gmane.org>
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 68aa923..dcb59c8 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -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.

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

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: avoid races with workqueue

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1007 bytes --]

Hi!

> >>>+++ b/drivers/leds/led-class.c
> >>>@@ -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.
> 
> 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.
									Pavel


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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH] leds: avoid races with workqueue

[Jacek Anaszewski]

On 5/2/19 9:13 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> +++ b/drivers/leds/led-class.c
>>>>> @@ -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.
>>
>> 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")

?
-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: avoid races with workqueue

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1657 bytes --]

On Thu 2019-05-02 21:28:06, Jacek Anaszewski wrote:
> On 5/2/19 9:13 PM, Pavel Machek wrote:
> >Hi!
> >
> >>>>>+++ b/drivers/leds/led-class.c
> >>>>>@@ -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.
> >>
> >>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")

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

Thanks!
									Pavel



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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH] leds: avoid races with workqueue

[Jacek Anaszewski]

On 5/2/19 10:06 PM, Pavel Machek wrote:
> On Thu 2019-05-02 21:28:06, Jacek Anaszewski wrote:
>> On 5/2/19 9:13 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>> +++ b/drivers/leds/led-class.c
>>>>>>> @@ -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.
>>>>
>>>> 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")
> 
> Yes, that looks right. If you can add fixes: and cc: stable tags, the
> rest should happen automagically.

Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org 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.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: avoid races with workqueue

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 970 bytes --]

Hi!

> >Yes, that looks right. If you can add fixes: and cc: stable tags, the
> >rest should happen automagically.
> 
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org 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.

Well, docs says Cc: stable is right way to request inclusion, and it
does not talk about Fixes:... but I guess that will work too.

Thanks,
									Pavel

(
To have the patch automatically included in the stable tree, add the
tag

.. code-block:: none

     Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

in the sign-off area. Once the patch is merged it will be applied to
the stable tree without anything else needing to be done by the author
or subsystem maintainer.
)

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 201 bytes --]

_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel