Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936476AbcKDQqO (ORCPT ); Fri, 4 Nov 2016 12:46:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49028 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932541AbcKDQqM (ORCPT ); Fri, 4 Nov 2016 12:46:12 -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> <9d28f3d3-1d3d-c670-a102-3e2698764fdd@samsung.com> Cc: linux-kernel@vger.kernel.org, Sakari Ailus , Pavel Machek , Andrew Lunn From: Hans de Goede Message-ID: <9367cc2f-5ce9-0443-bb87-66198d437061@redhat.com> Date: Fri, 4 Nov 2016 17:46:07 +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: <9d28f3d3-1d3d-c670-a102-3e2698764fdd@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.30]); Fri, 04 Nov 2016 16:46:12 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3210 Lines: 80 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. Regards, Hans