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

[Manivannan Sadhasivam <manisadhasivam.linux@gmail.com>]

On Wed, Jan 22, 2025 at 11:04:12AM +0000, Avri Altman wrote:
> > > To fix this issue, we use the existing `is_initialized` flag in the
> > > `clk_gating` structure to ensure that the spinlock is only used after
> > > it has been properly initialized. We check this flag before using the
> > > spinlock in the `ufshcd_setup_clocks` function.
> > >
> > > It was incorrect in the first place to call `setup_clocks()` before
> > > `ufshcd_init_clk_gating()`, and the introduction of the new lock
> > > unmasked this bug.
> > 
> > If calling setup_clocks() before ufshcd_init_clk_gating() is incorrect, why are
> > you not reordering it?
> > 
> > Checking for 'clk_gating.is_initialized' looks like a hack.
> Actually 'clk_gating.is_initialized' seems like the standard way to do this - see e.g. in hold and release.
> As for moving setup_clocks() around, I have some concerns about moving it out of ufshcd_hba_init.
> Having considered the alternatives, it seems that using 'clk_gating.is_initialized' ,
> despite its limitations, is the most practical solution we have.
> 
> I am open though for other suggestions.
> 

Looking at the code again, I think it is OK to have this fix for now. But
someone should spend some time to revisit the locking part in this driver.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

-- 
மணிவண்ணன் சதாசிவம்