| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <573D6ED7.6080307@samsung.com>
Date: Thu, 19 May 2016 09:44:23 +0200
From: Jacek Anaszewski <j.anaszewski@...sung.com>
To: Tony Makkiel <tony.makkiel@...ri.com>
Cc: linux-leds@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] led: core: Fix brightness setting upon hardware blinking
enabled
Hi Tony,
Thanks for detecting this. I've added also a modification of the comment
over brightness_set op definition in the linux/leds.h, improved the
commit message a bit more, and applied the patch to the
fixes-for-4.7-rc2 branch of linux-leds.git.
Thanks,
Jacek Anaszewski
On 05/18/2016 06:22 PM, Tony Makkiel wrote:
> Commit 76931edd54f8 ("leds: fix brightness changing when software blinking
> is active") changed the semantics of led_set_brightness() which according
> to the documentation should disable blinking on any brightness setting.
> Moreover it made it different for soft blink case, where it was allowed
> for blink brightness change, and for hardware blink case, where setting
> any brightness greater than 0 was ignored.
>
> While the change itself is against the documentation claims, it was driven
> also by the fact that timer trigger remained active after turning blinking
> off. Fixing that would have required major refactoring in the led-core,
> led-class, and led-triggers because of cyclic dependencies.
>
> Finally, it has been decided that allowing for brightness change during
> blinking is beneficial as it can be accomplished without disturbing
> blink rhythm.
>
> The change in brightness setting semantics will not affect existing
> LED class drivers that implement blink_set op thanks to the LED_BLINK_SW
> flag introduced by this patch. The flag state will be from now on checked
> in led_set_brightness() which will allow to distinguish between hardware
> and software blink mode. In the former case the control will be passed
> directly to the drivers which apply their semantics on brightness set,
> which is disable the blinking in case of most such drivers. New drivers
> will apply new semantics and just change the brightness while hardware
> blinking is on, if possible.
>
> Due to the later LED core improvements this patch can't be applied directly
> on the patch that originally introduced the problem, but to the later one,
> that touched the affected code.
>
> Fixes: f1e80c07416a ("leds: core: Add two new LED_BLINK_ flags")
>
> Signed-off-by: Tony Makkiel <tony.makkiel@...ri.com>
> Signed-off-by: Jacek Anaszewski <j.anaszewski@...sung.com>
> ---
> Documentation/leds/leds-class.txt | 4 ++--
> drivers/leds/led-core.c | 9 ++++++---
> include/linux/leds.h | 19 ++++++++++---------
> 3 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
> index d406d98..44f5e6b 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -74,8 +74,8 @@ blink_set() function (see <linux/leds.h>). To set an LED to blinking,
> however, it is better to use the API function led_blink_set(), as it
> will check and implement software fallback if necessary.
>
> -To turn off blinking again, use the API function led_brightness_set()
> -as that will not just set the LED brightness but also stop any software
> +To turn off blinking, use the API function led_brightness_set()
> +with brightness value LED_OFF, which should stop any software
> timers that may have been required for blinking.
>
> The blink_set() function should choose a user friendly blinking value
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 3495d5d..3bce448 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -53,11 +53,12 @@ static void led_timer_function(unsigned long data)
>
> if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
> led_set_brightness_nosleep(led_cdev, LED_OFF);
> + led_cdev->flags &= ~LED_BLINK_SW;
> return;
> }
>
> if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
> - led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> + led_cdev->flags &= ~(LED_BLINK_ONESHOT_STOP | LED_BLINK_SW);
> return;
> }
>
> @@ -151,6 +152,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
> return;
> }
>
> + led_cdev->flags |= LED_BLINK_SW;
> mod_timer(&led_cdev->blink_timer, jiffies + 1);
> }
>
> @@ -219,6 +221,7 @@ void led_stop_software_blink(struct led_classdev *led_cdev)
> del_timer_sync(&led_cdev->blink_timer);
> led_cdev->blink_delay_on = 0;
> led_cdev->blink_delay_off = 0;
> + led_cdev->flags &= ~LED_BLINK_SW;
> }
> EXPORT_SYMBOL_GPL(led_stop_software_blink);
>
> @@ -226,10 +229,10 @@ void led_set_brightness(struct led_classdev *led_cdev,
> enum led_brightness brightness)
> {
> /*
> - * In case blinking is on delay brightness setting
> + * If software blink is active, delay brightness setting
> * until the next timer tick.
> */
> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> + if (led_cdev->flags & LED_BLINK_SW) {
> /*
> * If we need to disable soft blinking delegate this to the
> * work queue task to avoid problems in case we are called
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index d2b1306..99b1f0b 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -42,15 +42,16 @@ struct led_classdev {
> #define LED_UNREGISTERING (1 << 1)
> /* Upper 16 bits reflect control information */
> #define LED_CORE_SUSPENDRESUME (1 << 16)
> -#define LED_BLINK_ONESHOT (1 << 17)
> -#define LED_BLINK_ONESHOT_STOP (1 << 18)
> -#define LED_BLINK_INVERT (1 << 19)
> -#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 20)
> -#define LED_BLINK_DISABLE (1 << 21)
> -#define LED_SYSFS_DISABLE (1 << 22)
> -#define LED_DEV_CAP_FLASH (1 << 23)
> -#define LED_HW_PLUGGABLE (1 << 24)
> -#define LED_PANIC_INDICATOR (1 << 25)
> +#define LED_BLINK_SW (1 << 17)
> +#define LED_BLINK_ONESHOT (1 << 18)
> +#define LED_BLINK_ONESHOT_STOP (1 << 19)
> +#define LED_BLINK_INVERT (1 << 20)
> +#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 21)
> +#define LED_BLINK_DISABLE (1 << 22)
> +#define LED_SYSFS_DISABLE (1 << 23)
> +#define LED_DEV_CAP_FLASH (1 << 24)
> +#define LED_HW_PLUGGABLE (1 << 25)
> +#define LED_PANIC_INDICATOR (1 << 26)
>
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
>
Powered by blists - more mailing lists