RE: [PATCH v2] scsi: ufs: core: Ensure clk_gating.lock is used only after initialization

[Avri Altman]

> > +       /*
> > +        * Initialize clk_gating.lock early since it is being used in
> > +        * ufshcd_setup_clocks()
> > +        */
> > +       if (ufshcd_is_clkgating_allowed(hba))
> 
> This checks if the UFSHCD_CAP_CLK_GATING flag is set, which is only done in a
> subset of the host drivers:
> 
> drivers/ufs/host/ufs-exynos.c:  hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> drivers/ufs/host/ufs-mediatek.c:        hba->caps |= UFSHCD_CAP_CLK_GATING;
> drivers/ufs/host/ufs-qcom.c:    hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> drivers/ufs/host/ufs-sprd.c:    hba->caps |= UFSHCD_CAP_CLK_GATING |
> 
> > +               spin_lock_init(&hba->clk_gating.lock);
> 
> Hence the spinlock is not uninitialized on all other host drivers.
Oh - Thanks for pointing this out.  I totally missed it.
At is appears when clock_gating was invented - 1ab27c9cf8b6 ("ufs: Add support for clock gating")
The below was added without checking if ufshcd_is_clkgating_allowed, or better yet - clk_gating.is_initialized:

@@ -4151,12 +4420,19 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
                                        clki->name, on ? "en" : "dis");
                }
        }
+       } else if (!ret && on) {
+               spin_lock_irqsave(hba->host->host_lock, flags);
+               hba->clk_gating.state = CLKS_ON;
+               spin_unlock_irqrestore(hba->host->host_lock, flags);
        }

Looks like this is the missing piece.
Will fix this in a 2nd patch.

Thanks,
Avri

> 
> > +
> >         err = ufshcd_hba_init(hba);
> >         if (err)
> >                 goto out_error;
> 
> 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