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@xxxxxxxxxx> - Mani -- மணிவண்ணன் சதாசிவம்
