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

[PATCH v3 0/3] 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, remove some of those calls around
host registers accesses, which needs no protection.

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


Changes compared to v2:
 - leave out changing ufshcd_tmc_handler

Changes compared to v1:
 - get rid of redundant locking (Bart)
 - leave out the HCE register

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

Avri Altman (3):
  scsi: ufs: core: Remove redundant host_lock calls around UTMRLDBR.
  scsi: ufs: core: Remove redundant host_lock calls around UTMRLCLR
  scsi: ufs: core: Remove redundant host_lock calls around UTRLCLR.

 drivers/ufs/core/ufshcd.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] scsi: ufs: core: Remove redundant host_lock calls around UTMRLDBR.

[Avri Altman]

There is no need to serialize single read/write calls to the host
controller registers. Remove the redundant host_lock calls that protect
access to the task management doorbell register: UTMRLDBR.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9e6d008f4ea4..faea4b294bdb 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;
 }
@@ -1263,7 +1265,6 @@ static u32 ufshcd_pending_cmds(struct ufs_hba *hba)
 static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 					u64 wait_timeout_us)
 {
-	unsigned long flags;
 	int ret = 0;
 	u32 tm_doorbell;
 	u32 tr_pending;
@@ -1271,7 +1272,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.
@@ -1292,7 +1292,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 +1303,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 +1312,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;
 }
@@ -7065,12 +7062,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);
 
-	/* 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);
 
+	/* send command to the controller */
+	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
+
 	ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_SEND);
 
 	/* wait until the task management command is completed */
-- 
2.25.1

Re: [PATCH v3 1/3] scsi: ufs: core: Remove redundant host_lock calls around UTMRLDBR.

[Bart Van Assche]

On 10/24/24 12:50 AM, Avri Altman wrote:
> There is no need to serialize single read/write calls to the host
> controller registers. Remove the redundant host_lock calls that protect
> access to the task management doorbell register: UTMRLDBR.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v3 2/3] scsi: ufs: core: Remove redundant host_lock calls around UTMRLCLR

[Avri Altman]

There is no need to serialize single read/write calls to the host
controller registers. Remove the redundant host_lock calls that protect
access to the task management request List cLear register: UTMRLCLR.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index faea4b294bdb..c2f44834062e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7012,14 +7012,11 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
 {
 	int err = 0;
 	u32 mask = 1 << tag;
-	unsigned long flags;
 
 	if (!test_bit(tag, &hba->outstanding_tasks))
 		goto out;
 
-	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_utmrl_clear(hba, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* poll for max. 1 sec to clear door bell register by h/w */
 	err = ufshcd_wait_for_register(hba,
-- 
2.25.1

[PATCH v3 3/3] scsi: ufs: core: Remove redundant host_lock calls around UTRLCLR.

[Avri Altman]

There is no need to serialize single read/write calls to the host
controller registers. Remove the redundant host_lock calls that protect
access to the request list cLear register: UTRLCLR.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c2f44834062e..5eefdc02d62b 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3076,7 +3076,6 @@ bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd)
 static int ufshcd_clear_cmd(struct ufs_hba *hba, u32 task_tag)
 {
 	u32 mask;
-	unsigned long flags;
 	int err;
 
 	if (hba->mcq_enabled) {
@@ -3096,9 +3095,7 @@ 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);
 	ufshcd_utrl_clear(hba, mask);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/*
 	 * wait for h/w to clear corresponding bit in door-bell.
-- 
2.25.1

Re: [PATCH v3 0/3] Untie the host lock entanglement - part 1

[Martin K. Petersen]

Avri,

> 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, remove some of those calls
> around host registers accesses, which needs no protection.
>
> Doing this in phases seems like a reasonable approach, given the
> myriad use of the host lock.

Applied to 6.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 0/3] Untie the host lock entanglement - part 1

[Martin K. Petersen]

On Thu, 24 Oct 2024 10:50:30 +0300, Avri Altman wrote:

> 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, remove some of those calls around
> host registers accesses, which needs no protection.
> 
> [...]

Applied to 6.13/scsi-queue, thanks!

[1/3] scsi: ufs: core: Remove redundant host_lock calls around UTMRLDBR.
      https://git.kernel.org/mkp/scsi/c/2b314e182caa
[2/3] scsi: ufs: core: Remove redundant host_lock calls around UTMRLCLR
      https://git.kernel.org/mkp/scsi/c/5824e18b3db4
[3/3] scsi: ufs: core: Remove redundant host_lock calls around UTRLCLR.
      https://git.kernel.org/mkp/scsi/c/2a330f16ad30

-- 
Martin K. Petersen	Oracle Linux Engineering