| Message ID | 20241021120313.493371-5-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Untie the host lock entanglement - part 1 | expand |
On 10/21/24 5:03 AM, Avri Altman wrote: > Use the host register lock to serialize access to the Host Controller > Enable (HCE) register as well, instead of the host_lock. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/ufs/core/ufshcd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 4eee737a4fd5..3cc8ffc6929f 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4795,9 +4795,9 @@ void ufshcd_hba_stop(struct ufs_hba *hba) > * 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); > + spin_lock_irqsave(&hba->reg_lock, flags); > ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); > - spin_unlock_irqrestore(hba->host->host_lock, flags); > + spin_unlock_irqrestore(&hba->reg_lock, flags); > > err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, > CONTROLLER_ENABLE, CONTROLLER_DISABLE, Hi Avri, How about proceeding as follows for ufshcd_hba_stop(): * Remove the comment above the ufshcd_writel() call and add a disable_irq() call instead. * Call enable_irq() after ufshcd_writel() has finished and before ufshcd_wait_for_register() is called. * Do not hold any lock around the ufshcd_writel() call. Although the legacy interrupt is disabled by some but not all ufshcd_hba_stop() callers, I think it is safe to nest disable_irq() calls. From kernel/irq/manage.c: void __disable_irq(struct irq_desc *desc) { if (!desc->depth++) irq_disable(desc); } Thanks, Bart.
> On 10/21/24 5:03 AM, Avri Altman wrote: > > Use the host register lock to serialize access to the Host Controller > > Enable (HCE) register as well, instead of the host_lock. > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > --- > > drivers/ufs/core/ufshcd.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index 4eee737a4fd5..3cc8ffc6929f 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -4795,9 +4795,9 @@ void ufshcd_hba_stop(struct ufs_hba *hba) > > * 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); > > + spin_lock_irqsave(&hba->reg_lock, flags); > > ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); > > - spin_unlock_irqrestore(hba->host->host_lock, flags); > > + spin_unlock_irqrestore(&hba->reg_lock, flags); > > > > err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, > > CONTROLLER_ENABLE, > > CONTROLLER_DISABLE, > > Hi Avri, > > How about proceeding as follows for ufshcd_hba_stop(): > * Remove the comment above the ufshcd_writel() call and add a > disable_irq() call instead. > * Call enable_irq() after ufshcd_writel() has finished and before > ufshcd_wait_for_register() is called. > * Do not hold any lock around the ufshcd_writel() call. > > Although the legacy interrupt is disabled by some but not all > ufshcd_hba_stop() callers, I think it is safe to nest disable_irq() calls. From > kernel/irq/manage.c: > > void __disable_irq(struct irq_desc *desc) { > if (!desc->depth++) > irq_disable(desc); > } Thanks. I think I'll exclude this one from this series. I want it to be clear that all instances are about removing redundant host_lock calls before register access. Here, it’s a bit different since need to verify that the UFS interrupt handler is not active on another CPU. I will follow your suggestion - but in another series. Thanks, Avri > > Thanks, > > Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 4eee737a4fd5..3cc8ffc6929f 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4795,9 +4795,9 @@ void ufshcd_hba_stop(struct ufs_hba *hba) * 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); + spin_lock_irqsave(&hba->reg_lock, flags); ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE); - spin_unlock_irqrestore(hba->host->host_lock, flags); + spin_unlock_irqrestore(&hba->reg_lock, flags); err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE, CONTROLLER_ENABLE, CONTROLLER_DISABLE,
Use the host register lock to serialize access to the Host Controller Enable (HCE) register as well, instead of the host_lock. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufshcd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)