| Message ID | 20241124110747.206651-1-avri.altman@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: core: Do not hold any lock in ufshcd_hba_stop | expand |
On 11/24/24 3:07 AM, Avri Altman wrote: > This change is motivated by Bart's suggestion in [1], which enables to > further reduce the scsi host lock usage in the ufs driver. The reason > why it make sense, because although the legacy interrupt is disabled by > some but not all ufshcd_hba_stop() callers, it is safe to nest > disable_irq() calls as it checks the irq depth. > > [1] https://lore.kernel.org/linux-scsi/c58e4fce-0a74-4469-ad16-f1edbd670728@acm.org/ > > Suggested-by: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/ufs/core/ufshcd.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index acc3607bbd9c..09a5ff49da5a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4811,16 +4811,11 @@ EXPORT_SYMBOL_GPL(ufshcd_make_hba_operational); > */ > void ufshcd_hba_stop(struct ufs_hba *hba) > { > - unsigned long flags; > int err; > > - /* > - * Obtain the host lock to prevent that the controller is disabled > - * while the UFS interrupt handler is active on another CPU. > - */ > - spin_lock_irqsave(hba->host->host_lock, flags); > + ufshcd_disable_irq(hba); > ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); > - spin_unlock_irqrestore(hba->host->host_lock, flags); > + ufshcd_enable_irq(hba); > > err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, > CONTROLLER_ENABLE, CONTROLLER_DISABLE, Shouldn't the ufshcd_enable_irq() call be moved below the ufshcd_wait_for_register() call? Otherwise a race condition could cause the interrupt handler to be triggered while the controller is being disabled. Thanks, Bart.
> Shouldn't the ufshcd_enable_irq() call be moved below the > ufshcd_wait_for_register() call? Otherwise a race condition could cause the > interrupt handler to be triggered while the controller is being disabled. Correct. if ufshcd_enable_irq() is called before the controller is fully disabled, it could allow interrupts to occur prematurely. Will move it. Thanks, Avri > > Thanks, > > Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index acc3607bbd9c..09a5ff49da5a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4811,16 +4811,11 @@ EXPORT_SYMBOL_GPL(ufshcd_make_hba_operational); */ void ufshcd_hba_stop(struct ufs_hba *hba) { - unsigned long flags; int err; - /* - * Obtain the host lock to prevent that the controller is disabled - * while the UFS interrupt handler is active on another CPU. - */ - spin_lock_irqsave(hba->host->host_lock, flags); + ufshcd_disable_irq(hba); ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); - spin_unlock_irqrestore(hba->host->host_lock, flags); + ufshcd_enable_irq(hba); err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, CONTROLLER_ENABLE, CONTROLLER_DISABLE,
This change is motivated by Bart's suggestion in [1], which enables to further reduce the scsi host lock usage in the ufs driver. The reason why it make sense, because although the legacy interrupt is disabled by some but not all ufshcd_hba_stop() callers, it is safe to nest disable_irq() calls as it checks the irq depth. [1] https://lore.kernel.org/linux-scsi/c58e4fce-0a74-4469-ad16-f1edbd670728@acm.org/ Suggested-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufshcd.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)