Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752187AbcKFOwt (ORCPT ); Sun, 6 Nov 2016 09:52:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39168 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752038AbcKFOwr (ORCPT ); Sun, 6 Nov 2016 09:52:47 -0500 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> <9d28f3d3-1d3d-c670-a102-3e2698764fdd@samsung.com> <9367cc2f-5ce9-0443-bb87-66198d437061@redhat.com> Cc: linux-kernel@vger.kernel.org, Sakari Ailus , Pavel Machek , Andrew Lunn From: Hans de Goede Message-ID: <1189fd14-a0a6-867e-1082-e84771f29014@redhat.com> Date: Sun, 6 Nov 2016 15:52:42 +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: <9367cc2f-5ce9-0443-bb87-66198d437061@redhat.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.25]); Sun, 06 Nov 2016 14:52:47 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3805 Lines: 92 Hi, On 04-11-16 17:46, Hans de Goede wrote: > Hi, > > On 04-11-16 17:06, Jacek Anaszewski wrote: >> Hi Hans, >> >> On 11/04/2016 12:53 PM, Hans de Goede wrote: >>> 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. >> >> led_update_brightness() currently has two users besides LED subsystem >> (at least grep reports those places): >> >> 1. v4l2-flash-led-class wrapper, for which led_access mutex was >> introduced. Its purpose was to disable LED sysfs interface while >> v4l2-flash wrapper takes over control of LED class device >> (not saying that the mutex wasn't missing even without this >> use case). Now I've realized that the call to >> led_sysfs_is_disabled() is missing in this patch. >> 2. /drivers/platform/x86/thinkpad_acpi.c - it calls >> led_update_brightness() on suspend >> >> I think that the best we can now do is to add >> lockdep_assert_held(&led_cdev->led_access) in led_update_brightness() >> and a description saying that the caller has to acquire led_access >> lock before calling it. Similarly as in case of >> led_sysfs_disable()/led_sysfs_disable(). > > The problem is not only callers of led_update_brightness() not holding > led_cdev->led_access, the problem is also callers of led_set_brightness > not holding led_cdev->led_access and they cannot take this lock because > they may be called from a non-blocking context. Thinking more about this, using a spinlock is also not going to work because led_cdev->brightness_set_blocking and led_cdev->brightness_get can both block and thus cannot be called with a spinlock held. I think that we need to just make this a problem of the led drivers and in include/linux/leds.h document that the led-core does not do locking and that the drivers themselves need to protect against their brightness_set / brightness_get callbacks when necessary. Regards, Hans