| Message ID | 20190429152259.GB10501@amd (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | leds: avoid races with workqueue | expand |
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@ucw.cz> > > 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)) >
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@ucw.cz> > > > >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
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@ucw.cz> >>> >>> 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.
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
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") ?
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
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@kernel.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.
Hi! > >Yes, that looks right. If you can add fixes: and cc: stable tags, the > >rest should happen automagically. > > Cc: stable@kernel.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@vger.kernel.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. )
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))
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@ucw.cz>