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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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