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

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

[Avri Altman]

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(-)

-- 
2.25.1

[PATCH 1/4] scsi: ufs: core: Introduce a new host register lock

[Avri Altman]

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.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 26 ++++++++++++++++++--------
 include/ufs/ufshcd.h      |  3 +++
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9e6d008f4ea4..21eda055fb7d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1245,11 +1245,13 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
 static u32 ufshcd_pending_cmds(struct ufs_hba *hba)
 {
 	const struct scsi_device *sdev;
+	unsigned long flags;
 	u32 pending = 0;
 
-	lockdep_assert_held(hba->host->host_lock);
+	spin_lock_irqsave(hba->host->host_lock, flags);
 	__shost_for_each_device(sdev, hba->host)
 		pending += sbitmap_weight(&sdev->budget_map);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	return pending;
 }
@@ -1271,7 +1273,6 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 	ktime_t start;
 
 	ufshcd_hold(hba);
-	spin_lock_irqsave(hba->host->host_lock, flags);
 	/*
 	 * Wait for all the outstanding tasks/transfer requests.
 	 * Verify by checking the doorbell registers are clear.
@@ -1283,7 +1284,10 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 			goto out;
 		}
 
+		spin_lock_irqsave(&hba->reg_lock, flags);
 		tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
+		spin_unlock_irqrestore(&hba->reg_lock, flags);
+
 		tr_pending = ufshcd_pending_cmds(hba);
 		if (!tm_doorbell && !tr_pending) {
 			timeout = false;
@@ -1292,7 +1296,6 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 			break;
 		}
 
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
 		io_schedule_timeout(msecs_to_jiffies(20));
 		if (ktime_to_us(ktime_sub(ktime_get(), start)) >
 		    wait_timeout_us) {
@@ -1304,7 +1307,6 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 			 */
 			do_last_check = true;
 		}
-		spin_lock_irqsave(hba->host->host_lock, flags);
 	} while (tm_doorbell || tr_pending);
 
 	if (timeout) {
@@ -1314,7 +1316,6 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 		ret = -EBUSY;
 	}
 out:
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_release(hba);
 	return ret;
 }
@@ -6881,9 +6882,13 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
 	irqreturn_t ret = IRQ_NONE;
 	int tag;
 
-	spin_lock_irqsave(hba->host->host_lock, flags);
+	spin_lock_irqsave(&hba->reg_lock, flags);
 	pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
+	spin_unlock_irqrestore(&hba->reg_lock, flags);
+
 	issued = hba->outstanding_tasks & ~pending;
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
 	for_each_set_bit(tag, &issued, hba->nutmrs) {
 		struct request *req = hba->tmf_rqs[tag];
 		struct completion *c = req->end_io_data;
@@ -7065,11 +7070,13 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	memcpy(hba->utmrdl_base_addr + task_tag, treq, sizeof(*treq));
 	ufshcd_vops_setup_task_mgmt(hba, task_tag, tm_function);
 
+	spin_unlock_irqrestore(host->host_lock, flags);
+
+	spin_lock_irqsave(&hba->reg_lock, flags);
 	/* send command to the controller */
 	__set_bit(task_tag, &hba->outstanding_tasks);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
-
-	spin_unlock_irqrestore(host->host_lock, flags);
+	spin_unlock_irqrestore(&hba->reg_lock, flags);
 
 	ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_SEND);
 
@@ -10469,6 +10476,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	sema_init(&hba->host_sem, 1);
 
+	/* Initialize host register read/write spinlock */
+	spin_lock_init(&hba->reg_lock);
+
 	/* Initialize UIC command mutex */
 	mutex_init(&hba->uic_cmd_mutex);
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a95282b9f743..6712833cc156 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -847,6 +847,7 @@ enum ufshcd_mcq_opr {
  * @host: Scsi_Host instance of the driver
  * @dev: device handle
  * @ufs_device_wlun: WLUN that controls the entire UFS device.
+ * @reg_lock: protect register reads/writes
  * @hwmon_device: device instance registered with the hwmon core.
  * @curr_dev_pwr_mode: active UFS device power mode.
  * @uic_link_state: active state of the link to the UFS device.
@@ -978,6 +979,8 @@ struct ufs_hba {
 	struct device *dev;
 	struct scsi_device *ufs_device_wlun;
 
+	spinlock_t reg_lock;
+
 #ifdef CONFIG_SCSI_UFS_HWMON
 	struct device *hwmon_device;
 #endif
-- 
2.25.1

Re: [PATCH 1/4] scsi: ufs: core: Introduce a new host register lock

[Bart Van Assche]

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.

Thanks,

Bart.

RE: [PATCH 1/4] scsi: ufs: core: Introduce a new host register lock

[Avri Altman]

> 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.

[PATCH 2/4] scsi: ufs: core: Use reg_lock to protect UTMRLCLR

[Avri Altman]

Use the host register lock to serialize access to the UTMRLCLR 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 21eda055fb7d..081cbf7174da 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7025,9 +7025,9 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
 	if (!test_bit(tag, &hba->outstanding_tasks))
 		goto out;
 
-	spin_lock_irqsave(hba->host->host_lock, flags);
+	spin_lock_irqsave(&hba->reg_lock, flags);
 	ufshcd_utmrl_clear(hba, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	spin_unlock_irqrestore(&hba->reg_lock, flags);
 
 	/* poll for max. 1 sec to clear door bell register by h/w */
 	err = ufshcd_wait_for_register(hba,
-- 
2.25.1

Re: [PATCH 2/4] scsi: ufs: core: Use reg_lock to protect UTMRLCLR

[Bart Van Assche]

On 10/21/24 5:03 AM, Avri Altman wrote:
>   	if (!test_bit(tag, &hba->outstanding_tasks))
>   		goto out;
>   
> -	spin_lock_irqsave(hba->host->host_lock, flags);
> +	spin_lock_irqsave(&hba->reg_lock, flags);
>   	ufshcd_utmrl_clear(hba, tag);
> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	spin_unlock_irqrestore(&hba->reg_lock, flags);
>   
>   	/* poll for max. 1 sec to clear door bell register by h/w */
>   	err = ufshcd_wait_for_register(hba,

Hi Avri,

ufshcd_utmrl_clear() performs a single write so I assume that calls of
that function do not have to be serialized?

Thanks,

Bart.

RE: [PATCH 2/4] scsi: ufs: core: Use reg_lock to protect UTMRLCLR

[Avri Altman]

> 
> On 10/21/24 5:03 AM, Avri Altman wrote:
> >       if (!test_bit(tag, &hba->outstanding_tasks))
> >               goto out;
> >
> > -     spin_lock_irqsave(hba->host->host_lock, flags);
> > +     spin_lock_irqsave(&hba->reg_lock, flags);
> >       ufshcd_utmrl_clear(hba, tag);
> > -     spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +     spin_unlock_irqrestore(&hba->reg_lock, flags);
> >
> >       /* poll for max. 1 sec to clear door bell register by h/w */
> >       err = ufshcd_wait_for_register(hba,
> 
> Hi Avri,
> 
> ufshcd_utmrl_clear() performs a single write so I assume that calls of that
> function do not have to be serialized?
Done.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

[PATCH 3/4] scsi: ufs: core: Use reg_lock to protect UTRLCLR

[Avri Altman]

Use the host register lock to serialize access to the UTRLCLR 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 081cbf7174da..4eee737a4fd5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -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.
-- 
2.25.1

Re: [PATCH 3/4] scsi: ufs: core: Use reg_lock to protect UTRLCLR

[Bart Van Assche]

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.

Thanks,

Bart.

RE: [PATCH 3/4] scsi: ufs: core: Use reg_lock to protect UTRLCLR

[Avri Altman]

> 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.

[PATCH 4/4] scsi: ufs: core: Use reg_lock to protect HCE register

[Avri Altman]

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,
-- 
2.25.1

Re: [PATCH 4/4] scsi: ufs: core: Use reg_lock to protect HCE register

[Bart Van Assche]

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.

RE: [PATCH 4/4] scsi: ufs: core: Use reg_lock to protect HCE register

[Avri Altman]

> 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.