Messages in this thread | | | Subject | Re: [PATCH] leds: Add mutex protection in brightness_show() | From | Hans de Goede <> | Date | Fri, 4 Nov 2016 12:53:17 +0100 |
| |
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.anaszewski@samsung.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Andrew Lunn <andrew@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
| |