| Message ID | 20210215151405.2551143-1-geert+renesas@glider.be |
|---|---|
| State | Superseded |
| Headers | show |
| Series | staging: board: Fix uninitialized spinlock when attaching genpd | expand |
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
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
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
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
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,
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(-)