[PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[Geert Uytterhoeven]

On Armadillo-800-EVA with CONFIG_DEBUG_SPINLOCK=y:

    BUG: spinlock bad magic on CPU#0, swapper/1
     lock: lcdc0_device+0x10c/0x308, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
    CPU: 0 PID: 1 Comm: swapper Not tainted 5.11.0-rc5-armadillo-00036-gbbca04be7a80-dirty #287
    Hardware name: Generic R8A7740 (Flattened Device Tree)
    [<c010c3c8>] (unwind_backtrace) from [<c010a49c>] (show_stack+0x10/0x14)
    [<c010a49c>] (show_stack) from [<c0159534>] (do_raw_spin_lock+0x20/0x94)
    [<c0159534>] (do_raw_spin_lock) from [<c040858c>] (dev_pm_get_subsys_data+0x8c/0x11c)
    [<c040858c>] (dev_pm_get_subsys_data) from [<c05fbcac>] (genpd_add_device+0x78/0x2b8)
    [<c05fbcac>] (genpd_add_device) from [<c0412db4>] (of_genpd_add_device+0x34/0x4c)
    [<c0412db4>] (of_genpd_add_device) from [<c0a1ea74>] (board_staging_register_device+0x11c/0x148)
    [<c0a1ea74>] (board_staging_register_device) from [<c0a1eac4>] (board_staging_register_devices+0x24/0x28)

of_genpd_add_device() is called before platform_device_register(), as it
needs to attach the genpd before the device is probed.  But the spinlock
is only initialized when the device is registered.

Fix this by open-coding the spinlock initialization, cfr.
device_pm_init_common() in the internal drivers/base code, and in the
SuperH early platform code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Exposed by fw_devlinks changing probe order.
Masked before due to an unrelated wait context check failure, which
disabled any further spinlock checks.
https://lore.kernel.org/linux-acpi/CAMuHMdVL-1RKJ5u-HDVA4F4w_+8yGvQQuJQBcZMsdV4yXzzfcw@mail.gmail.com
---
 drivers/staging/board/board.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
index cb6feb34dd401ae3..604612937f038e92 100644
--- a/drivers/staging/board/board.c
+++ b/drivers/staging/board/board.c
@@ -136,6 +136,7 @@ int __init board_staging_register_clock(const struct board_staging_clk *bsc)
 static int board_staging_add_dev_domain(struct platform_device *pdev,
 					const char *domain)
 {
+	struct device *dev = &pdev->dev;
 	struct of_phandle_args pd_args;
 	struct device_node *np;
 
@@ -148,7 +149,11 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
 	pd_args.np = np;
 	pd_args.args_count = 0;
 
-	return of_genpd_add_device(&pd_args, &pdev->dev);
+	/* Cfr. device_pm_init_common() */
+	spin_lock_init(&dev->power.lock);
+	dev->power.early_init = true;
+
+	return of_genpd_add_device(&pd_args, dev);
 }
 #else
 static inline int board_staging_add_dev_domain(struct platform_device *pdev,
-- 
2.25.1

Re: [PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[Saravana Kannan]

On Mon, Feb 15, 2021 at 7:14 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> On Armadillo-800-EVA with CONFIG_DEBUG_SPINLOCK=y:
>
>     BUG: spinlock bad magic on CPU#0, swapper/1
>      lock: lcdc0_device+0x10c/0x308, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
>     CPU: 0 PID: 1 Comm: swapper Not tainted 5.11.0-rc5-armadillo-00036-gbbca04be7a80-dirty #287
>     Hardware name: Generic R8A7740 (Flattened Device Tree)
>     [<c010c3c8>] (unwind_backtrace) from [<c010a49c>] (show_stack+0x10/0x14)
>     [<c010a49c>] (show_stack) from [<c0159534>] (do_raw_spin_lock+0x20/0x94)
>     [<c0159534>] (do_raw_spin_lock) from [<c040858c>] (dev_pm_get_subsys_data+0x8c/0x11c)
>     [<c040858c>] (dev_pm_get_subsys_data) from [<c05fbcac>] (genpd_add_device+0x78/0x2b8)
>     [<c05fbcac>] (genpd_add_device) from [<c0412db4>] (of_genpd_add_device+0x34/0x4c)
>     [<c0412db4>] (of_genpd_add_device) from [<c0a1ea74>] (board_staging_register_device+0x11c/0x148)
>     [<c0a1ea74>] (board_staging_register_device) from [<c0a1eac4>] (board_staging_register_devices+0x24/0x28)
>
> of_genpd_add_device() is called before platform_device_register(), as it
> needs to attach the genpd before the device is probed.  But the spinlock
> is only initialized when the device is registered.
>
> Fix this by open-coding the spinlock initialization, cfr.
> device_pm_init_common() in the internal drivers/base code, and in the
> SuperH early platform code.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Exposed by fw_devlinks changing probe order.
> Masked before due to an unrelated wait context check failure, which
> disabled any further spinlock checks.
> https://lore.kernel.org/linux-acpi/CAMuHMdVL-1RKJ5u-HDVA4F4w_+8yGvQQuJQBcZMsdV4yXzzfcw@mail.gmail.com
> ---
>  drivers/staging/board/board.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
> index cb6feb34dd401ae3..604612937f038e92 100644
> --- a/drivers/staging/board/board.c
> +++ b/drivers/staging/board/board.c
> @@ -136,6 +136,7 @@ int __init board_staging_register_clock(const struct board_staging_clk *bsc)
>  static int board_staging_add_dev_domain(struct platform_device *pdev,
>                                         const char *domain)
>  {
> +       struct device *dev = &pdev->dev;
>         struct of_phandle_args pd_args;
>         struct device_node *np;
>
> @@ -148,7 +149,11 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
>         pd_args.np = np;
>         pd_args.args_count = 0;
>
> -       return of_genpd_add_device(&pd_args, &pdev->dev);
> +       /* Cfr. device_pm_init_common() */

What's Cfr?

> +       spin_lock_init(&dev->power.lock);
> +       dev->power.early_init = true;

Also, I tried looking up, but it's not exactly what this flag
represents other than the fact the spinlock has been initialized?
Which is weird to me. So maybe Rafael can double check this?

-Saravana

> +
> +       return of_genpd_add_device(&pd_args, dev);
>  }
>  #else
>  static inline int board_staging_add_dev_domain(struct platform_device *pdev,
> --
> 2.25.1
>

Re: [PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[Geert Uytterhoeven]

Hi Saravana,

On Mon, Feb 15, 2021 at 7:37 PM Saravana Kannan <saravanak@google.com> wrote:
> On Mon, Feb 15, 2021 at 7:14 AM Geert Uytterhoeven
> > @@ -148,7 +149,11 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
> >         pd_args.np = np;
> >         pd_args.args_count = 0;
> >
> > -       return of_genpd_add_device(&pd_args, &pdev->dev);
> > +       /* Cfr. device_pm_init_common() */
>
> What's Cfr?

"compare to" (from Latin "confer").

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

Re: [PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[Saravana Kannan]

On Mon, Feb 15, 2021 at 11:10 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Mon, Feb 15, 2021 at 7:37 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Mon, Feb 15, 2021 at 7:14 AM Geert Uytterhoeven
> > > @@ -148,7 +149,11 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
> > >         pd_args.np = np;
> > >         pd_args.args_count = 0;
> > >
> > > -       return of_genpd_add_device(&pd_args, &pdev->dev);
> > > +       /* Cfr. device_pm_init_common() */
> >
> > What's Cfr?
>
> "compare to" (from Latin "confer").

Can you please change this to "refer to" or "similar to"? Also, not
sure if this comment is even adding anything useful even if you switch
the words.

Also, device_pm_init_common() is used in two places outside of
drivers/base/ with this change. Maybe better to move it to
linux/device.h?

-Saravana

Re: [PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[Geert Uytterhoeven]

Hi Saravana,

On Mon, Feb 15, 2021 at 10:03 PM Saravana Kannan <saravanak@google.com> wrote:
> On Mon, Feb 15, 2021 at 11:10 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Mon, Feb 15, 2021 at 7:37 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Mon, Feb 15, 2021 at 7:14 AM Geert Uytterhoeven
> > > > @@ -148,7 +149,11 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
> > > >         pd_args.np = np;
> > > >         pd_args.args_count = 0;
> > > >
> > > > -       return of_genpd_add_device(&pd_args, &pdev->dev);
> > > > +       /* Cfr. device_pm_init_common() */
> > >
> > > What's Cfr?
> >
> > "compare to" (from Latin "confer").
>
> Can you please change this to "refer to" or "similar to"? Also, not
> sure if this comment is even adding anything useful even if you switch
> the words.

I changed it to "Initialization similar to device_pm_init_common()"

> Also, device_pm_init_common() is used in two places outside of
> drivers/base/ with this change. Maybe better to move it to
> linux/device.h?

arch/sh/drivers/platform_early.c has a separate definition, and this
is intentional, cfr. commit 507fd01d53333387 ("drivers: move the early
platform device support to arch/sh"):

    In order not to export internal drivers/base functions to arch code for
    this temporary solution - copy the two needed routines for driver
    matching from drivers/base/platform.c to arch/sh/drivers/platform_early.c.

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

Re: [PATCH] staging: board: Fix uninitialized spinlock when attaching genpd

[Saravana Kannan]

On Thu, Feb 25, 2021 at 1:25 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Mon, Feb 15, 2021 at 10:03 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Mon, Feb 15, 2021 at 11:10 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Mon, Feb 15, 2021 at 7:37 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Mon, Feb 15, 2021 at 7:14 AM Geert Uytterhoeven
> > > > > @@ -148,7 +149,11 @@ static int board_staging_add_dev_domain(struct platform_device *pdev,
> > > > >         pd_args.np = np;
> > > > >         pd_args.args_count = 0;
> > > > >
> > > > > -       return of_genpd_add_device(&pd_args, &pdev->dev);
> > > > > +       /* Cfr. device_pm_init_common() */
> > > >
> > > > What's Cfr?
> > >
> > > "compare to" (from Latin "confer").
> >
> > Can you please change this to "refer to" or "similar to"? Also, not
> > sure if this comment is even adding anything useful even if you switch
> > the words.
>
> I changed it to "Initialization similar to device_pm_init_common()"
>
> > Also, device_pm_init_common() is used in two places outside of
> > drivers/base/ with this change. Maybe better to move it to
> > linux/device.h?
>
> arch/sh/drivers/platform_early.c has a separate definition, and this
> is intentional, cfr. commit 507fd01d53333387 ("drivers: move the early
> platform device support to arch/sh"):
>
>     In order not to export internal drivers/base functions to arch code for
>     this temporary solution - copy the two needed routines for driver
>     matching from drivers/base/platform.c to arch/sh/drivers/platform_early.c.
>

Thanks. The comments and decision to copy the code sounds okay to me.
But I'll still leave the Ack/Review to Rafael or someone else as I'm
not too familiar with the intent of this flag.

-Saravana