| Message ID | 20250126064521.3857557-3-avri.altman@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for UFS clock gating initialization | expand |
On Sun, 2025-01-26 at 08:45 +0200, Avri Altman wrote: > } else if (!ret && on && hba->clk_gating.is_initialized) { > scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) > hba->clk_gating.state = CLKS_ON; > trace_ufshcd_clk_gating(dev_name(hba->dev) Consolidating all clock-related initialization into a single function, such as ufshcd_init_clocks(), should be considered.
> On Sun, 2025-01-26 at 08:45 +0200, Avri Altman wrote: > > } else if (!ret && on && hba->clk_gating.is_initialized) { > > scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) > > hba->clk_gating.state = CLKS_ON; > > trace_ufshcd_clk_gating(dev_name(hba->dev) > > Consolidating all clock-related initialization into a single function, such as > ufshcd_init_clocks(), should be considered. Hi Bean, I don't think it will resolve the issue that is addressed in this patch. However, consolidating all clock-related initialization into a single function, is indeed a good idea. I might try to do that - but in another patch. Thanks, Avri
Hi Avri, Thanks for your patch! On Sun, 26 Jan 2025 at 07:48, Avri Altman <avri.altman@wdc.com> wrote: > This commit addresses an issue where `clk_gating.state` is being toggled > in `ufshcd_setup_clocks` even if clock gating is not allowed. This can > lead to a crash with the following error: > > BUG: spinlock bad magic on CPU#6, swapper/0/1 > lock: 0xffffff84443014e8, .magic: 00000000, .owner: <none>/-1, > .owner_cpu: 0 > CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted > 6.13.0-rcar3-initrd-08318-g75abbef32a94 #896 > Hardware name: R-Car S4 Starter Kit board (DT) > Call trace: > show_stack+0x18/0x24 (C) > dump_stack_lvl+0x60/0x80 > dump_stack+0x18/0x24 > spin_bug+0x7c/0xa0 > do_raw_spin_lock+0x34/0xb4 > _raw_spin_lock_irqsave+0x1c/0x30 > class_spinlock_irqsave_constructor+0x18/0x30 > ufshcd_setup_clocks+0x98/0x23c > ufshcd_init+0x288/0xd38 > ufshcd_pltfrm_init+0x618/0x738 > ufs_renesas_probe+0x18/0x24 > platform_probe+0x68/0xb8 > really_probe+0x138/0x268 > __driver_probe_device+0xf4/0x10c > driver_probe_device+0x3c/0xf8 > __driver_attach+0xf0/0x100 > bus_for_each_dev+0x84/0xdc > driver_attach+0x24/0x30 > bus_add_driver+0xe8/0x1dc > driver_register+0xbc/0xf8 > __platform_driver_register+0x24/0x30 > ufs_renesas_platform_init+0x1c/0x28 > do_one_initcall+0x84/0x1f4 > kernel_init_freeable+0x238/0x23c > kernel_init+0x20/0x120 > ret_from_fork+0x10/0x20 Note that after "[PATCH v3 1/2] scsi: ufs: core: Ensure clk_gating.lock is used only after initialization", I no longer see the above BUG(), so I don't think it's a good idea to include this log. > The root cause of the issue is that `clk_gating.state` is being toggled > even if clock gating is not allowed. This can lead to the spinlock being > used before it is properly initialized. Which doesn't mean the underlying issue no longer exists... > The fix is to add a check for `hba->clk_gating.is_initialized` before > toggling `clk_gating.state` in `ufshcd_setup_clocks`. Since > `clk_gating.lock` is now initialized unconditionally, this is for > documentation purposes, to ensure clarity in the code. The primary fix > remains to prevent toggling the `clk_gating.state` if clock gating is > not allowed. > > Fixes: 1ab27c9cf8b6 ("ufs: Add support for clock gating") > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Avri Altman <avri.altman@wdc.com> Seems to work fine on R-Car S4 SK, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> 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
> Hi Avri, > > Thanks for your patch! > > On Sun, 26 Jan 2025 at 07:48, Avri Altman <avri.altman@wdc.com> wrote: > > This commit addresses an issue where `clk_gating.state` is being > > toggled in `ufshcd_setup_clocks` even if clock gating is not allowed. > > This can lead to a crash with the following error: > > > > BUG: spinlock bad magic on CPU#6, swapper/0/1 > > lock: 0xffffff84443014e8, .magic: 00000000, .owner: <none>/-1, > > .owner_cpu: 0 > > CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted > > 6.13.0-rcar3-initrd-08318-g75abbef32a94 #896 > > Hardware name: R-Car S4 Starter Kit board (DT) > > Call trace: > > show_stack+0x18/0x24 (C) > > dump_stack_lvl+0x60/0x80 > > dump_stack+0x18/0x24 > > spin_bug+0x7c/0xa0 > > do_raw_spin_lock+0x34/0xb4 > > _raw_spin_lock_irqsave+0x1c/0x30 > > class_spinlock_irqsave_constructor+0x18/0x30 > > ufshcd_setup_clocks+0x98/0x23c > > ufshcd_init+0x288/0xd38 > > ufshcd_pltfrm_init+0x618/0x738 > > ufs_renesas_probe+0x18/0x24 > > platform_probe+0x68/0xb8 > > really_probe+0x138/0x268 > > __driver_probe_device+0xf4/0x10c > > driver_probe_device+0x3c/0xf8 > > __driver_attach+0xf0/0x100 > > bus_for_each_dev+0x84/0xdc > > driver_attach+0x24/0x30 > > bus_add_driver+0xe8/0x1dc > > driver_register+0xbc/0xf8 > > __platform_driver_register+0x24/0x30 > > ufs_renesas_platform_init+0x1c/0x28 > > do_one_initcall+0x84/0x1f4 > > kernel_init_freeable+0x238/0x23c > > kernel_init+0x20/0x120 > > ret_from_fork+0x10/0x20 > > Note that after "[PATCH v3 1/2] scsi: ufs: core: Ensure clk_gating.lock is used > only after initialization", I no longer see the above BUG(), so I don't think it's > a good idea to include this log. Done. Will remove. > > > The root cause of the issue is that `clk_gating.state` is being > > toggled even if clock gating is not allowed. This can lead to the > > spinlock being used before it is properly initialized. > > Which doesn't mean the underlying issue no longer exists... Done. Will rephrase. Thanks, Avri > > > The fix is to add a check for `hba->clk_gating.is_initialized` before > > toggling `clk_gating.state` in `ufshcd_setup_clocks`. Since > > `clk_gating.lock` is now initialized unconditionally, this is for > > documentation purposes, to ensure clarity in the code. The primary fix > > remains to prevent toggling the `clk_gating.state` if clock gating is > > not allowed. > > > > Fixes: 1ab27c9cf8b6 ("ufs: Add support for clock gating") > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > Seems to work fine on R-Car S4 SK, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > 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
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index b73c87da383d..abe0774133f5 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -9140,7 +9140,7 @@ static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) if (!IS_ERR_OR_NULL(clki->clk) && clki->enabled) clk_disable_unprepare(clki->clk); } - } else if (!ret && on) { + } else if (!ret && on && hba->clk_gating.is_initialized) { scoped_guard(spinlock_irqsave, &hba->clk_gating.lock) hba->clk_gating.state = CLKS_ON; trace_ufshcd_clk_gating(dev_name(hba->dev),
This commit addresses an issue where `clk_gating.state` is being toggled in `ufshcd_setup_clocks` even if clock gating is not allowed. This can lead to a crash with the following error: BUG: spinlock bad magic on CPU#6, swapper/0/1 lock: 0xffffff84443014e8, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0 CPU: 6 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.13.0-rcar3-initrd-08318-g75abbef32a94 #896 Hardware name: R-Car S4 Starter Kit board (DT) Call trace: show_stack+0x18/0x24 (C) dump_stack_lvl+0x60/0x80 dump_stack+0x18/0x24 spin_bug+0x7c/0xa0 do_raw_spin_lock+0x34/0xb4 _raw_spin_lock_irqsave+0x1c/0x30 class_spinlock_irqsave_constructor+0x18/0x30 ufshcd_setup_clocks+0x98/0x23c ufshcd_init+0x288/0xd38 ufshcd_pltfrm_init+0x618/0x738 ufs_renesas_probe+0x18/0x24 platform_probe+0x68/0xb8 really_probe+0x138/0x268 __driver_probe_device+0xf4/0x10c driver_probe_device+0x3c/0xf8 __driver_attach+0xf0/0x100 bus_for_each_dev+0x84/0xdc driver_attach+0x24/0x30 bus_add_driver+0xe8/0x1dc driver_register+0xbc/0xf8 __platform_driver_register+0x24/0x30 ufs_renesas_platform_init+0x1c/0x28 do_one_initcall+0x84/0x1f4 kernel_init_freeable+0x238/0x23c kernel_init+0x20/0x120 ret_from_fork+0x10/0x20 The root cause of the issue is that `clk_gating.state` is being toggled even if clock gating is not allowed. This can lead to the spinlock being used before it is properly initialized. The fix is to add a check for `hba->clk_gating.is_initialized` before toggling `clk_gating.state` in `ufshcd_setup_clocks`. Since `clk_gating.lock` is now initialized unconditionally, this is for documentation purposes, to ensure clarity in the code. The primary fix remains to prevent toggling the `clk_gating.state` if clock gating is not allowed. Fixes: 1ab27c9cf8b6 ("ufs: Add support for clock gating") Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufshcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)