Hi, On 04-11-16 08:52, Jacek Anaszewski wrote:
Initially the claim about no need for lock in brightness_show() was valid as the function was just returning unchanged LED brightness. After the addition of led_update_brightness() this is no longer true, as the function can change the brightness if a LED class driver implements brightness_get op. It can lead to races between led_update_brightness() and led_set_brightness(), resulting in overwriting new brightness with the old one before the former is written to the device.Signed-off-by: Jacek Anaszewski <j.anaszew...@samsung.com> Cc: Hans de Goede <hdego...@redhat.com> Cc: Sakari Ailus <sakari.ai...@linux.intel.com> Cc: Pavel Machek <pa...@ucw.cz> Cc: Andrew Lunn <and...@lunn.ch> --- drivers/leds/led-class.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 731e4eb..0c2307b 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -30,8 +30,9 @@ static ssize_t brightness_show(struct device *dev, { struct led_classdev *led_cdev = dev_get_drvdata(dev); - /* no lock needed for this */ + mutex_lock(&led_cdev->led_access); led_update_brightness(led_cdev); + mutex_unlock(&led_cdev->led_access); return sprintf(buf, "%u\n", led_cdev->brightness); }
I'm afraid that this fix is not enough, the led_access lock is only held when the brightness is being updated through sysfs, not for trigger / sw-blinking updates (which cannot take a mutex as they may be called from non blocking contexts). We may need to consider to add a spinlock to the led_classdev and always lock that when calling into the driver, except for when the driver has a brightness_set_blocking callback. Which will need special handling. Regards, Hans
