mbox series

[0/4] Untie the host lock entanglement - part 1

Message ID 20241021120313.493371-1-avri.altman@wdc.com
Headers show
Series Untie the host lock entanglement - part 1 | expand

Message

Avri Altman Oct. 21, 2024, 12:03 p.m. UTC 
While trying to simplify the ufs core driver with the guard() macro [1],
Bart made note of the abuse of the scsi host lock in the ufs driver.
Indeed, the host lock is deeply entangled in various flows across the
driver, as if it was some occasional default synchronization mean.

Here is the first part of defusing it, replace some of its occurrences
of host registers accesses, with a dedicated host register lock. 

Doing this in phases seems like a reasonable approach, given the myriad
use.


[1] https://lore.kernel.org/linux-scsi/0b031b8f-c07c-42ef-af93-7336439d3c37@acm.org/

Avri Altman (4):
  scsi: ufs: core: Introduce a new host register lock
  scsi: ufs: core: Use reg_lock to protect UTMRLCLR
  scsi: ufs: core: Use reg_lock to protect UTRLCLR
  scsi: ufs: core: Use reg_lock to protect HCE register

 drivers/ufs/core/ufshcd.c | 38 ++++++++++++++++++++++++--------------
 include/ufs/ufshcd.h      |  3 +++
 2 files changed, 27 insertions(+), 14 deletions(-)

Comments

Avri Altman Oct. 22, 2024, 6:02 a.m. UTC | #1 
> On 10/21/24 5:03 AM, Avri Altman wrote:
> > Introduce a new host register read/write lock.  Use it to protect
> > access to the task management doorbell register: UTMRLDBR.  This is
> > not the UTRLDBR which is already protected by its own outstanding_lock.
> 
> Why is this necessary? I think it is allowed to submit host controller reads
> and writes simultaneously from different threads. Only read/modify/writes
> have to be serialized.
Done.

> 
> Thanks,
> 
> Bart.
Avri Altman Oct. 22, 2024, 6:03 a.m. UTC | #2 
> On 10/21/24 5:03 AM, Avri Altman wrote:
> > @@ -3100,9 +3100,9 @@ static int ufshcd_clear_cmd(struct ufs_hba *hba,
> u32 task_tag)
> >       mask = 1U << task_tag;
> >
> >       /* clear outstanding transaction before retry */
> > -     spin_lock_irqsave(hba->host->host_lock, flags);
> > +     spin_lock_irqsave(&hba->reg_lock, flags);
> >       ufshcd_utrl_clear(hba, mask);
> > -     spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +     spin_unlock_irqrestore(&hba->reg_lock, flags);
> >
> >       /*
> >        * wait for h/w to clear corresponding bit in door-bell.
> 
> Hi Avri,
> 
> A similar comment as for the previous patch applies to this patch:
> ufshcd_utrl_clear() performs a single MMIO write so I don't think that calls of
> this function have to be serialized.
Done.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.