Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
Hi!
> >This fixes one problem:
> >Signed-off-by: Pavel Machek
> >
> >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(_cdev->set_brightness_work)) {
> >+printk("Waiting for brightness set to finish\n");
> >+schedule();
> >+}
>
> Or even better:
>
> flush_work(_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(_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(_cdev->set_brightness_work);
if (!test_bit(LED_BLINK_ONESHOT, _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(_list_lock);
unlock:
+ flush_work(_cdev->set_brightness_work);
mutex_unlock(_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, _cdev->blink_delay_on,
_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", _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,
Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
On 4/28/19 12:32 AM, Pavel Machek wrote:
Hi!
This fixes one problem:
Pavel
Signed-off-by: Pavel Machek
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(_cdev->set_brightness_work)) {
+ printk("Waiting for brightness set to finish\n");
+ schedule();
+ }
Or even better:
flush_work(_cdev->set_brightness_work);
if (!test_bit(LED_BLINK_ONESHOT, _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
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
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, _cdev->blink_delay_on,
_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 :02:00.0 eth1: 10/100 speed: disabling TSO
[ 145.887931] timer_trig_activate
[ 145.888011] LED set 0 to 2
[ 145.79] LED set 0 to 2... 0
[ 145.93] 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, _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, _cdev->blink_delay_on,
_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", _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",
Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
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=151622365715313=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 = _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 signature.asc Description: Digital signature ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
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=151622365715313=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 ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
Hi!
> Kernel 5.1.0-rc1 + some unrelated bits.
I tried adding
https://marc.info/?l=linux-kernel=151622365715313=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
signature.asc
Description: Digital signature
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] Thinkpad ACPI led -- it keeps blinking
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=151622365715313=2 -- Best regards, Jacek Anaszewski ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
