Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934294AbcKDLxY (ORCPT ); Fri, 4 Nov 2016 07:53:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51192 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933734AbcKDLxW (ORCPT ); Fri, 4 Nov 2016 07:53:22 -0400 Subject: Re: [PATCH] leds: Add mutex protection in brightness_show() To: Jacek Anaszewski , linux-leds@vger.kernel.org References: <1478245962-15706-1-git-send-email-j.anaszewski@samsung.com> Cc: linux-kernel@vger.kernel.org, Sakari Ailus , Pavel Machek , Andrew Lunn From: Hans de Goede Message-ID: Date: Fri, 4 Nov 2016 12:53:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1478245962-15706-1-git-send-email-j.anaszewski@samsung.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 04 Nov 2016 11:53:21 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1850 Lines: 51 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 > Cc: Hans de Goede > Cc: Sakari Ailus > Cc: Pavel Machek > Cc: Andrew Lunn > --- > 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