[PATCH v3b] regulator: core: avoid unused variable warning

[PATCH v3b] regulator: core: avoid unused variable warning

[Arnd Bergmann]

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 <arnd@arndb.de>
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++);
-		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);
 }
 
 /**

[PATCH v3b] regulator: core: avoid unused variable warning

[Geert Uytterhoeven]

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:
[<c0247b84>] 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: [<c0042934>] process_one_work+0x1c0/0x3dc
 #1:  (deferred_probe_work){+.+.+.}, at: [<c0042934>]
process_one_work+0x1c0/0x3dc
 #2:  (&dev->mutex){......}, at: [<c02a1464>] __device_attach+0x1c/0x104
 #3:  (&_host->ios_lock){+.+...}, at: [<c038798c>] 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
[<c00173a0>] (unwind_backtrace) from [<c0013094>] (show_stack+0x10/0x14)
[<c0013094>] (show_stack) from [<c01f2338>] (dump_stack+0x70/0x8c)
[<c01f2338>] (dump_stack) from [<c0067c70>]
(print_unlock_imbalance_bug+0xa4/0xd8)
[<c0067c70>] (print_unlock_imbalance_bug) from [<c006d0e0>]
(lock_release+0x1c0/0x38c)
[<c006d0e0>] (lock_release) from [<c04ab954>]
(__mutex_unlock_slowpath+0x110/0x190)
[<c04ab954>] (__mutex_unlock_slowpath) from [<c0247b84>]
(regulator_set_voltage+0x38/0x50)
[<c0247b84>] (regulator_set_voltage) from [<c03767a0>]
(mmc_regulator_set_ocr+0x40/0xc4)
[<c03767a0>] (mmc_regulator_set_ocr) from [<c0387a98>]
(tmio_mmc_set_ios+0x140/0x1d8)
[<c0387a98>] (tmio_mmc_set_ios) from [<c0377a6c>] (mmc_power_up+0x3c/0xb0)
[<c0377a6c>] (mmc_power_up) from [<c037878c>] (mmc_start_host+0x50/0x7c)
[<c037878c>] (mmc_start_host) from [<c0379754>] (mmc_add_host+0x5c/0x80)
[<c0379754>] (mmc_add_host) from [<c0388438>] (tmio_mmc_host_probe+0x3dc/0x48c)
[<c0388438>] (tmio_mmc_host_probe) from [<c0389bc8>]
(sh_mobile_sdhi_probe+0x1c4/0x3e8)
[<c0389bc8>] (sh_mobile_sdhi_probe) from [<c02a2e30>]
(platform_drv_probe+0x50/0xa0)
[<c02a2e30>] (platform_drv_probe) from [<c02a1684>]
(driver_probe_device+0x110/0x28c)
[<c02a1684>] (driver_probe_device) from [<c029ff74>]
(bus_for_each_drv+0x84/0x94)
[<c029ff74>] (bus_for_each_drv) from [<c02a14d8>] (__device_attach+0x90/0x104)
[<c02a14d8>] (__device_attach) from [<c02a0c28>] (bus_probe_device+0x28/0x84)
[<c02a0c28>] (bus_probe_device) from [<c02a1040>]
(deferred_probe_work_func+0x60/0x88)
[<c02a1040>] (deferred_probe_work_func) from [<c00429ac>]
(process_one_work+0x238/0x3dc)
[<c00429ac>] (process_one_work) from [<c00430ac>] (worker_thread+0x2a8/0x3e8)
[<c00430ac>] (worker_thread) from [<c0047874>] (kthread+0xd8/0xec)
[<c0047874>] (kthread) from [<c000fb48>] (ret_from_fork+0x14/0x2c)

On Fri, Nov 20, 2015 at 3:24 PM, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>
> 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.

>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3b] regulator: core: avoid unused variable warning

[Arnd Bergmann]

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:
> [<c0247b84>] 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: [<c0042934>] process_one_work+0x1c0/0x3dc
>  #1:  (deferred_probe_work){+.+.+.}, at: [<c0042934>]
> process_one_work+0x1c0/0x3dc
>  #2:  (&dev->mutex){......}, at: [<c02a1464>] __device_attach+0x1c/0x104
>  #3:  (&_host->ios_lock){+.+...}, at: [<c038798c>] 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
> [<c00173a0>] (unwind_backtrace) from [<c0013094>] (show_stack+0x10/0x14)
> [<c0013094>] (show_stack) from [<c01f2338>] (dump_stack+0x70/0x8c)
> [<c01f2338>] (dump_stack) from [<c0067c70>]
> (print_unlock_imbalance_bug+0xa4/0xd8)
> [<c0067c70>] (print_unlock_imbalance_bug) from [<c006d0e0>]
> (lock_release+0x1c0/0x38c)
> [<c006d0e0>] (lock_release) from [<c04ab954>]
> (__mutex_unlock_slowpath+0x110/0x190)
> [<c04ab954>] (__mutex_unlock_slowpath) from [<c0247b84>]
> (regulator_set_voltage+0x38/0x50)
> [<c0247b84>] (regulator_set_voltage) from [<c03767a0>]
> (mmc_regulator_set_ocr+0x40/0xc4)
> [<c03767a0>] (mmc_regulator_set_ocr) from [<c0387a98>]
> (tmio_mmc_set_ios+0x140/0x1d8)
> [<c0387a98>] (tmio_mmc_set_ios) from [<c0377a6c>] (mmc_power_up+0x3c/0xb0)
> [<c0377a6c>] (mmc_power_up) from [<c037878c>] (mmc_start_host+0x50/0x7c)
> [<c037878c>] (mmc_start_host) from [<c0379754>] (mmc_add_host+0x5c/0x80)
> [<c0379754>] (mmc_add_host) from [<c0388438>] (tmio_mmc_host_probe+0x3dc/0x48c)
> [<c0388438>] (tmio_mmc_host_probe) from [<c0389bc8>]
> (sh_mobile_sdhi_probe+0x1c4/0x3e8)
> [<c0389bc8>] (sh_mobile_sdhi_probe) from [<c02a2e30>]
> (platform_drv_probe+0x50/0xa0)
> [<c02a2e30>] (platform_drv_probe) from [<c02a1684>]
> (driver_probe_device+0x110/0x28c)
> [<c02a1684>] (driver_probe_device) from [<c029ff74>]
> (bus_for_each_drv+0x84/0x94)
> [<c029ff74>] (bus_for_each_drv) from [<c02a14d8>] (__device_attach+0x90/0x104)
> [<c02a14d8>] (__device_attach) from [<c02a0c28>] (bus_probe_device+0x28/0x84)
> [<c02a0c28>] (bus_probe_device) from [<c02a1040>]
> (deferred_probe_work_func+0x60/0x88)
> [<c02a1040>] (deferred_probe_work_func) from [<c00429ac>]
> (process_one_work+0x238/0x3dc)
> [<c00429ac>] (process_one_work) from [<c00430ac>] (worker_thread+0x2a8/0x3e8)
> [<c00430ac>] (worker_thread) from [<c0047874>] (kthread+0xd8/0xec)
> [<c0047874>] (kthread) from [<c000fb48>] (ret_from_fork+0x14/0x2c)
> 
> On Fri, Nov 20, 2015 at 3:24 PM, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>
> > 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:
> [<c0247b84>] 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 <geert@linux-m68k.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
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);
 }
 

[PATCH v3b] regulator: core: avoid unused variable warning

[Mark Brown]

On Fri, Nov 27, 2015 at 10:36:35AM +0100, Arnd Bergmann wrote:
> On Friday 27 November 2015 10:25:11 Geert Uytterhoeven wrote:

> 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

Please submit this normally, it looks OK.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151127/8e71439a/attachment.sig>