linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3b] regulator: core: avoid unused variable warning
@ 2015-11-20 14:24 Arnd Bergmann
  2015-11-20 18:20 ` Applied "regulator: core: avoid unused variable warning" to the regulator tree Mark Brown
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Arnd Bergmann @ 2015-11-20 14:24 UTC (permalink / raw)
  To: Mark Brown
  Cc: Sascha Hauer, Liam Girdwood, linux-kernel, linux-arm-kernel,
	Peter Zijlstra, Ingo Molnar

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);
 }
 
 /**


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Applied "regulator: core: avoid unused variable warning" to the regulator tree
  2015-11-20 14:24 [PATCH v3b] regulator: core: avoid unused variable warning Arnd Bergmann
@ 2015-11-20 18:20 ` Mark Brown
  2015-11-27  9:25 ` [PATCH v3b] regulator: core: avoid unused variable warning Geert Uytterhoeven
  2015-12-01 23:00 ` Applied "regulator: core: avoid unused variable warning" " Mark Brown
  2 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2015-11-20 18:20 UTC (permalink / raw)
  To: Arnd Bergmann, Mark Brown; +Cc: linux-kernel

The patch

   regulator: core: avoid unused variable warning

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From fa731ac7ea04a7d3a5c6d2f568132478c02a83b3 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 20 Nov 2015 15:24:39 +0100
Subject: [PATCH] regulator: core: avoid unused variable warning

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
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 73b7683355cd..c70017d5f74b 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);
 }
 
 /**
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3b] regulator: core: avoid unused variable warning
  2015-11-20 14:24 [PATCH v3b] regulator: core: avoid unused variable warning Arnd Bergmann
  2015-11-20 18:20 ` Applied "regulator: core: avoid unused variable warning" to the regulator tree Mark Brown
@ 2015-11-27  9:25 ` Geert Uytterhoeven
  2015-11-27  9:36   ` Arnd Bergmann
  2015-12-01 23:00 ` Applied "regulator: core: avoid unused variable warning" " Mark Brown
  2 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2015-11-27  9:25 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mark Brown, Sascha Hauer, Liam Girdwood, linux-kernel,
	linux-arm-kernel, Peter Zijlstra, Ingo Molnar

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@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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3b] regulator: core: avoid unused variable warning
  2015-11-27  9:25 ` [PATCH v3b] regulator: core: avoid unused variable warning Geert Uytterhoeven
@ 2015-11-27  9:36   ` Arnd Bergmann
  2015-11-27 11:05     ` Mark Brown
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Arnd Bergmann @ 2015-11-27  9:36 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Sascha Hauer, Liam Girdwood, linux-kernel,
	linux-arm-kernel, Peter Zijlstra, Ingo Molnar

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);
 }
 



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3b] regulator: core: avoid unused variable warning
  2015-11-27  9:36   ` Arnd Bergmann
@ 2015-11-27 11:05     ` Mark Brown
  2015-11-27 17:07     ` Applied "regulator: core: fix regulator_lock_supply regression" to the regulator tree Mark Brown
  2015-12-01 23:00     ` Mark Brown
  2 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2015-11-27 11:05 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Geert Uytterhoeven, Sascha Hauer, Liam Girdwood, linux-kernel,
	linux-arm-kernel, Peter Zijlstra, Ingo Molnar

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Applied "regulator: core: fix regulator_lock_supply regression" to the regulator tree
  2015-11-27  9:36   ` Arnd Bergmann
  2015-11-27 11:05     ` Mark Brown
@ 2015-11-27 17:07     ` Mark Brown
  2015-12-01 23:00     ` Mark Brown
  2 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2015-11-27 17:07 UTC (permalink / raw)
  To: Geert Uytterhoeven, Arnd Bergmann, Mark Brown; +Cc: linux-kernel

The patch

   regulator: core: fix regulator_lock_supply regression

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From bb41897e38c53458a88b271f2fbcd905ee1f9584 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 27 Nov 2015 14:46:41 +0100
Subject: [PATCH] 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")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c70017d5f74b..daffff83ced2 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);
 }
 
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Applied "regulator: core: fix regulator_lock_supply regression" to the regulator tree
  2015-11-27  9:36   ` Arnd Bergmann
  2015-11-27 11:05     ` Mark Brown
  2015-11-27 17:07     ` Applied "regulator: core: fix regulator_lock_supply regression" to the regulator tree Mark Brown
@ 2015-12-01 23:00     ` Mark Brown
  2 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2015-12-01 23:00 UTC (permalink / raw)
  To: Geert Uytterhoeven, Arnd Bergmann, Mark Brown; +Cc: linux-kernel

The patch

   regulator: core: fix regulator_lock_supply regression

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From bb41897e38c53458a88b271f2fbcd905ee1f9584 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 27 Nov 2015 14:46:41 +0100
Subject: [PATCH] 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")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c70017d5f74b..daffff83ced2 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);
 }
 
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Applied "regulator: core: avoid unused variable warning" to the regulator tree
  2015-11-20 14:24 [PATCH v3b] regulator: core: avoid unused variable warning Arnd Bergmann
  2015-11-20 18:20 ` Applied "regulator: core: avoid unused variable warning" to the regulator tree Mark Brown
  2015-11-27  9:25 ` [PATCH v3b] regulator: core: avoid unused variable warning Geert Uytterhoeven
@ 2015-12-01 23:00 ` Mark Brown
  2 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2015-12-01 23:00 UTC (permalink / raw)
  To: Arnd Bergmann, Mark Brown; +Cc: linux-kernel

The patch

   regulator: core: avoid unused variable warning

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From fa731ac7ea04a7d3a5c6d2f568132478c02a83b3 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 20 Nov 2015 15:24:39 +0100
Subject: [PATCH] regulator: core: avoid unused variable warning

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
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 73b7683355cd..c70017d5f74b 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);
 }
 
 /**
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-12-01 23:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20 14:24 [PATCH v3b] regulator: core: avoid unused variable warning Arnd Bergmann
2015-11-20 18:20 ` Applied "regulator: core: avoid unused variable warning" to the regulator tree Mark Brown
2015-11-27  9:25 ` [PATCH v3b] regulator: core: avoid unused variable warning Geert Uytterhoeven
2015-11-27  9:36   ` Arnd Bergmann
2015-11-27 11:05     ` Mark Brown
2015-11-27 17:07     ` Applied "regulator: core: fix regulator_lock_supply regression" to the regulator tree Mark Brown
2015-12-01 23:00     ` Mark Brown
2015-12-01 23:00 ` Applied "regulator: core: avoid unused variable warning" " Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).