From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Fri, 27 Nov 2015 10:36:35 +0100 Subject: [PATCH v3b] regulator: core: avoid unused variable warning In-Reply-To: References: <4083086.03MkICpY7S@wuerfel> Message-ID: <6486349.NfasOOYIHJ@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday 27 November 2015 10:25:11 Geert Uytterhoeven wrote: > Hi Arnd, Mark, > > I saw this BUG a few times lately, but it doesn't trigger on every boot: > > ===================================== > [ BUG: bad unlock balance detected! ] > 4.4.0-rc2-koelsch-02027-g82cc3c313143199b-dirty #2084 Tainted: G W > ------------------------------------- > kworker/u4:0/6 is trying to release lock (&rdev->mutex) at: > [] regulator_set_voltage+0x38/0x50 > but there are no more locks to release! > > other info that might help us debug this: > 4 locks held by kworker/u4:0/6: > #0: ("%s""deferwq"){++++.+}, at: [] process_one_work+0x1c0/0x3dc > #1: (deferred_probe_work){+.+.+.}, at: [] > process_one_work+0x1c0/0x3dc > #2: (&dev->mutex){......}, at: [] __device_attach+0x1c/0x104 > #3: (&_host->ios_lock){+.+...}, at: [] tmio_mmc_set_ios+0x34/0x1d8 > > stack backtrace: > CPU: 0 PID: 6 Comm: kworker/u4:0 Tainted: G W > 4.4.0-rc2-koelsch-02027-g82cc3c313143199b-dirty #2084 > Hardware name: Generic R8A7791 (Flattened Device Tree) > Workqueue: deferwq deferred_probe_work_func > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x70/0x8c) > [] (dump_stack) from [] > (print_unlock_imbalance_bug+0xa4/0xd8) > [] (print_unlock_imbalance_bug) from [] > (lock_release+0x1c0/0x38c) > [] (lock_release) from [] > (__mutex_unlock_slowpath+0x110/0x190) > [] (__mutex_unlock_slowpath) from [] > (regulator_set_voltage+0x38/0x50) > [] (regulator_set_voltage) from [] > (mmc_regulator_set_ocr+0x40/0xc4) > [] (mmc_regulator_set_ocr) from [] > (tmio_mmc_set_ios+0x140/0x1d8) > [] (tmio_mmc_set_ios) from [] (mmc_power_up+0x3c/0xb0) > [] (mmc_power_up) from [] (mmc_start_host+0x50/0x7c) > [] (mmc_start_host) from [] (mmc_add_host+0x5c/0x80) > [] (mmc_add_host) from [] (tmio_mmc_host_probe+0x3dc/0x48c) > [] (tmio_mmc_host_probe) from [] > (sh_mobile_sdhi_probe+0x1c4/0x3e8) > [] (sh_mobile_sdhi_probe) from [] > (platform_drv_probe+0x50/0xa0) > [] (platform_drv_probe) from [] > (driver_probe_device+0x110/0x28c) > [] (driver_probe_device) from [] > (bus_for_each_drv+0x84/0x94) > [] (bus_for_each_drv) from [] (__device_attach+0x90/0x104) > [] (__device_attach) from [] (bus_probe_device+0x28/0x84) > [] (bus_probe_device) from [] > (deferred_probe_work_func+0x60/0x88) > [] (deferred_probe_work_func) from [] > (process_one_work+0x238/0x3dc) > [] (process_one_work) from [] (worker_thread+0x2a8/0x3e8) > [] (worker_thread) from [] (kthread+0xd8/0xec) > [] (kthread) from [] (ret_from_fork+0x14/0x2c) > > On Fri, Nov 20, 2015 at 3:24 PM, Arnd Bergmann wrote: > > The second argument of the mutex_lock_nested() helper is only > > evaluated if CONFIG_DEBUG_LOCK_ALLOC is set. Otherwise we > > get this build warning for the new regulator_lock_supply > > function: > > > > drivers/regulator/core.c: In function 'regulator_lock_supply': > > drivers/regulator/core.c:142:6: warning: unused variable 'i' [-Wunused-variable] > > > > To avoid the warning, this restructures the code to make it > > both simpler and to move the 'i++' outside of the mutex_lock_nested > > call, where it is now always used and the variable is not > > flagged as unused. > > > > We had some discussion about changing mutex_lock_nested to an > > inline function, which would make the code do the right thing here, > > but in the end decided against it, in order to guarantee that > > mutex_lock_nested() does not introduced overhead without > > CONFIG_DEBUG_LOCK_ALLOC. > > > > Signed-off-by: Arnd Bergmann > > Fixes: 9f01cd4a915 ("regulator: core: introduce function to lock regulators and its supplies") > > Link: http://permalink.gmane.org/gmane.linux.kernel/2068900 > > --- > > This is a different approach I came up with now, feel free to > > pick either v3a or v3b of the patch, whichever seems more appropriate > > to you. > > > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > > index 4cf1390784e5..c9bdca5f3b9b 100644 > > --- a/drivers/regulator/core.c > > +++ b/drivers/regulator/core.c > > @@ -138,18 +138,10 @@ static bool have_full_constraints(void) > > */ > > static void regulator_lock_supply(struct regulator_dev *rdev) > > { > > - struct regulator *supply; > > - int i = 0; > > - > > - while (1) { > > - mutex_lock_nested(&rdev->mutex, i++); > > The above line was always executed at least once... > > > - supply = rdev->supply; > > - > > - if (!rdev->supply) > > - return; > > + int i; > > > > - rdev = supply->rdev; > > - } > > + for (i = 0; rdev->supply; rdev = rdev->supply->rdev, i++) > > + mutex_lock_nested(&rdev->mutex, i); > > ... but not anymore. > Damn, I was trying to be too smart. I tried to come up with the perfect line that did the same thing as before, but clearly failed. We could scrap that patch and take the other alternative that I sent (which is more obvious than this one), or add do it like this: 8<--- Subject: regulator: core: fix regulator_lock_supply regression As noticed by Geert Uytterhoeven, my patch to avoid a harmless build warning in regulator_lock_supply() was total crap and introduced a real bug: > [ BUG: bad unlock balance detected! ] > kworker/u4:0/6 is trying to release lock (&rdev->mutex) at: > [] regulator_set_voltage+0x38/0x50 we still lock the regulator supplies, but not the actual regulators, so we are missing a lock, and the unlock is unbalanced. This rectifies it by first locking the regulator device itself before using the same loop as before to lock its supplies. Reported-by: Geert Uytterhoeven Signed-off-by: Arnd Bergmann Fixes: 716fec9d1965 ("[SUBMITTED] regulator: core: avoid unused variable warning") diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index c9bdca5f3b9b..89e4dcb84758 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -140,7 +140,8 @@ static void regulator_lock_supply(struct regulator_dev *rdev) { int i; - for (i = 0; rdev->supply; rdev = rdev->supply->rdev, i++) + mutex_lock(&rdev->mutex); + for (i = 1; rdev->supply; rdev = rdev->supply->rdev, i++) mutex_lock_nested(&rdev->mutex, i); }