| 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: <5600FEB9.10409@samsung.com>
Date: Tue, 22 Sep 2015 09:09:45 +0200
From: Jacek Anaszewski <j.anaszewski@...sung.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
andrew@...n.ch
Subject: Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC
brightness setting
Hi Sakari,
Thanks for the review.
Just to avoid a confusion - this patch set depends on [1], which is
not merged yet and also needs a review. I reimplemented and split [2]
into two patch sets, so that [1] contained only modifications required
for removing work queues from LED class drivers, but yesterday decided
to send also the remaining part.
[1] https://lkml.org/lkml/2015/9/16/299
[2] https://lkml.org/lkml/2015/8/20/426
On 09/22/2015 08:42 AM, Sakari Ailus wrote:
> Hi Jacek,
>
> Thanks for the patchset. A few comments below.
>
> Jacek Anaszewski wrote:
>> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
>> led_set_brightness now calls led_set_brightness_nosleep instead of
>> choosing between sync and async op basing on the flags defined by the
>> driver.
>>
>> From now on, if a user wants to make sure that brightness will be set
>> synchronously, they have to use led_set_brightness_sync API. It is now
>> being made publicly available since it has become apparent that it is
>> a caller who should decide whether brightness is to be set in
>> a synchronous or an asynchronous way.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@...sung.com>
>> ---
>> drivers/leds/led-class-flash.c | 4 ----
>> drivers/leds/led-class.c | 2 --
>> drivers/leds/led-core.c | 33 +++++++++++++++++++--------------
>> drivers/leds/leds.h | 13 -------------
>> include/linux/leds.h | 19 ++++++++++++++++---
>> 5 files changed, 35 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
>> index 300a2c9..f53783b 100644
>> --- a/drivers/leds/led-class-flash.c
>> +++ b/drivers/leds/led-class-flash.c
>> @@ -316,10 +316,6 @@ int led_classdev_flash_register(struct device *parent,
>> if (ret < 0)
>> return ret;
>>
>> - /* Setting a torch brightness needs to have immediate effect */
>> - led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
>> - led_cdev->flags |= SET_BRIGHTNESS_SYNC;
>> -
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(led_classdev_flash_register);
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 7385f98..37eec1a 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -215,8 +215,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>> if (!led_cdev->max_brightness)
>> led_cdev->max_brightness = LED_FULL;
>>
>> - led_cdev->flags |= SET_BRIGHTNESS_ASYNC;
>> -
>> led_update_brightness(led_cdev);
>>
>> led_init_core(led_cdev);
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index d8649f1..d0eb838 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -215,8 +215,6 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
>> void led_set_brightness(struct led_classdev *led_cdev,
>> enum led_brightness brightness)
>> {
>> - int ret = 0;
>> -
>> /*
>> * In case blinking is on delay brightness setting
>> * until the next timer tick.
>> @@ -235,19 +233,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
>> led_cdev->blink_brightness = brightness;
>> }
>> return;
>> - }
>> -
>> - if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>> + } else {
>
> Useless use of else.
Right.
>> led_set_brightness_nosleep(led_cdev, brightness);
>> - return;
>> - } else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
>> - ret = led_set_brightness_sync(led_cdev, brightness);
>> - else
>> - ret = -EINVAL;
>> -
>> - if (ret < 0)
>> - dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
>> - ret);
>> + }
>> }
>> EXPORT_SYMBOL(led_set_brightness);
>>
>> @@ -269,6 +257,23 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>> }
>> EXPORT_SYMBOL(led_set_brightness_nosleep);
>>
>> +int led_set_brightness_sync(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + WARN_ON(led_cdev->blink_delay_on || led_cdev->blink_delay_off);
>
> How about simply returning an error instead?
Indeed, it will be more relevant here.
>> +
>> + led_cdev->brightness = min(value, led_cdev->max_brightness);
>> +
>> + if (led_cdev->flags & LED_SUSPENDED)
>> + return 0;
>> +
>> + if (led_cdev->brightness_set_blocking)
>> + return led_cdev->brightness_set_blocking(led_cdev,
>> + led_cdev->brightness);
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(led_set_brightness_sync);
>> +
>> int led_update_brightness(struct led_classdev *led_cdev)
>> {
>> int ret = 0;
>> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
>> index 04b8e41..62d9eba 100644
>> --- a/drivers/leds/leds.h
>> +++ b/drivers/leds/leds.h
>> @@ -16,19 +16,6 @@
>> #include <linux/rwsem.h>
>> #include <linux/leds.h>
>>
>> -static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
>> - enum led_brightness value)
>> -{
>> - int ret = 0;
>> -
>> - led_cdev->brightness = min(value, led_cdev->max_brightness);
>> -
>> - if (!(led_cdev->flags & LED_SUSPENDED))
>> - ret = led_cdev->brightness_set_blocking(led_cdev,
>> - led_cdev->brightness);
>> - return ret;
>> -}
>> -
>> static inline int led_get_brightness(struct led_classdev *led_cdev)
>> {
>> return led_cdev->brightness;
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index ae3c178..2019929 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -47,9 +47,7 @@ struct led_classdev {
>> #define LED_BLINK_CHANGE (1 << 20)
>> #define LED_BLINK_DISABLE (1 << 21)
>> #define LED_SYSFS_DISABLE (1 << 22)
>> -#define SET_BRIGHTNESS_ASYNC (1 << 23)
>> -#define SET_BRIGHTNESS_SYNC (1 << 24)
>> -#define LED_DEV_CAP_FLASH (1 << 25)
>> +#define LED_DEV_CAP_FLASH (1 << 23)
>>
>> /* Set LED brightness level */
>> /* Must not sleep, use a workqueue if needed */
>> @@ -159,6 +157,21 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
>> */
>> extern void led_set_brightness(struct led_classdev *led_cdev,
>> enum led_brightness brightness);
>> +
>> +/**
>> + * led_set_brightness_sync - set LED brightness synchronously
>> + * @led_cdev: the LED to set
>> + * @brightness: the brightness to set it to
>> + *
>> + * Set an LED's brightness immediately. This function will block
>> + * the caller for the time required for accessing device register,
>> + * and it can sleep.
>> + *
>> + * Returns: 0 on success or negative error value on failure
>> + */
>> +extern int led_set_brightness_sync(struct led_classdev *led_cdev,
>> + enum led_brightness value);
>> +
>> /**
>> * led_update_brightness - update LED brightness
>> * @led_cdev: the LED to query
>>
>
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists