Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964904AbcKDQHG (ORCPT ); Fri, 4 Nov 2016 12:07:06 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:25388 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932299AbcKDQHE (ORCPT ); Fri, 4 Nov 2016 12:07:04 -0400 X-AuditID: cbfec7f4-f791c6d000006eac-84-581cb2228e92 Subject: Re: [PATCH] leds: Add mutex protection in brightness_show() To: Hans de Goede , linux-leds@vger.kernel.org Cc: linux-kernel@vger.kernel.org, Sakari Ailus , Pavel Machek , Andrew Lunn From: Jacek Anaszewski Message-id: <9d28f3d3-1d3d-c670-a102-3e2698764fdd@samsung.com> Date: Fri, 04 Nov 2016 17:06:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupkleLIzCtJLcpLzFFi42LZduznOV2lTTIRBmsXqlqcv3uI2eLN8elM Fpd3zWGz2PpmHaPF3VNH2Sw+bfnG5MDmMe9koMfOHZ+ZPN7vu8rmsWL1d3aPz5vkAlijuGxS UnMyy1KL9O0SuDLmL//IUnBfrGLfoyfsDYzHhLoYOTkkBEwknj3sY4KwxSQu3FvP1sXIxSEk sJRRYs3j6ewQzmdGiT8PNjLCdMyb/pQZxBYSWMYoseeBIkTRM0aJN5Nugo0SFnCRmDrvCViD iICzRFfnBiaQImaBHkaJW8+nsYAk2AQMJX6+eA3WwCtgJ3F4xkvWLkYODhYBVYnZ8xRATFGB CIndd1MhKgQlfky+xwIS5gSqXt+dABJmFnCUeLBoJyuELS+xec1bZpBNEgLz2CXWbHoLVi8h ICux6QAzhOkiMe+lNMQnwhKvjm9hh7BlJC5P7maBaJ3MKHHx2E1WCGc1o8TGzk4WiCpriYb/ v1gglvFJTNo2HWoor0RHGzRAPSSmbvjJBmE7Sry/9gcaoBcYJR7++cU+gVF+FpJ3ZiH5YRaS HxYwMq9iFEktLc5NTy020StOzC0uzUvXS87P3cQITCCn/x3/soNx8TGrQ4wCHIxKPLw7pslE CLEmlhVX5h5ilOBgVhLhrV8PFOJNSaysSi3Kjy8qzUktPsQozcGiJM67Z8GVcCGB9MSS1OzU 1ILUIpgsEwenVANjEffEa11FU+N7SmtkrI9c/3Kd+Z/KjiqOQ60iJzdNv/Ft0sUWnYZvPSxR J0Ll5vw9FNslF/k41oP/Wla6mONtxbV3/O5qTqia94hxidzi18dvRW7sVJ2X+1bkSXHxd5fr G881KAfszViZeih7v+q6gOm8Z+5GzdltqsR5ZhGb015ZdrOUrTlblFiKMxINtZiLihMBXr0N sRwDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrNIsWRmVeSWpSXmKPExsVy+t/xa7rMm2QiDCZ3Klucv3uI2eLN8elM Fpd3zWGz2PpmHaPF3VNH2Sw+bfnG5MDmMe9koMfOHZ+ZPN7vu8rmsWL1d3aPz5vkAlij3Gwy UhNTUosUUvOS81My89JtlUJD3HQtlBTyEnNTbZUidH1DgpQUyhJzSoE8IwM04OAc4B6spG+X 4JYxf/lHloL7YhX7Hj1hb2A8JtTFyMkhIWAiMW/6U2YIW0ziwr31bF2MXBxCAksYJe4s+gzl PGOUuPyzlRWkSljARWLqvCeMILaIgLNEV+cGJoiiC0AdO3uYQRxmgS5GieU3p4JVsQkYSvx8 8ZoJxOYVsJM4POMl0CQODhYBVYnZ8xRAwqICERK3Vn1khCgRlPgx+R4LSAknUPn67gSQMLOA rcSC9+tYIGx5ic1r3jJPYBSYhaRjFpKyWUjKFjAyr2IUSS0tzk3PLTbSK07MLS7NS9dLzs/d xAiMqG3Hfm7Zwdj1LvgQowAHoxIP745pMhFCrIllxZW5hxglOJiVRHjr1wOFeFMSK6tSi/Lj i0pzUosPMZoCvTCRWUo0OR8Y7Xkl8YYmhuaWhkbGFhbmRkZK4rxTP1wJFxJITyxJzU5NLUgt gulj4uCUamBM+12SVG3Sm1hxdvVtZQlmp0+ZdWsuzZZZytFo37/4l0Wcd9Zb+ZNcDuv/pYpp siZ+L2Wz7F5l8fbSlgkFmxKexUmzHJrQ1Mj6+0tvV1eGlE7wjW8SV6wXhocaz0xK/P12Q2FJ tblduKe3j84u5+s/7s6ZUcyn+yy/jevhnrLrR50n8f61YFRiKc5INNRiLipOBACYWlBlvgIA AA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20161104160658eucas1p28df8ef75cd4bfc09289229773a9beefd X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?SmFjZWsgQW5hc3pld3NraRtTUlBPTC1TeXN0ZW0gRlcgIChN?= =?UTF-8?B?Qikb7IK87ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?SmFjZWsgQW5hc3pld3NraRtTUlBPTC1TeXN0ZW0gRlcgIChN?= =?UTF-8?B?QikbU2Ftc3VuZyBFbGVjdHJvbmljcxtTZW5pb3IgU29mdHdhcmUgRW5naW5l?= =?UTF-8?B?ZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjc1MjY=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20161104115324epcas3p32bee0b0f57ae8d1141ded4fa1d1f2ae2 X-RootMTR: 20161104115324epcas3p32bee0b0f57ae8d1141ded4fa1d1f2ae2 References: <1478245962-15706-1-git-send-email-j.anaszewski@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2839 Lines: 72 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(). -- Best regards, Jacek Anaszewski