diff mbox series

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

Message ID 20241022074319.512127-2-avri.altman@wdc.com (mailing list archive)
State Superseded
Headers show
Series Untie the host lock entanglement - part 1 | expand

Commit Message

Avri Altman Oct. 22, 2024, 7:43 a.m. UTC 
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 | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Bart Van Assche Oct. 22, 2024, 4:51 p.m. UTC | #1 
On 10/22/24 12:43 AM, Avri Altman wrote:
> @@ -6877,13 +6874,13 @@ static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba, u32 intr_status)
>    */
>   static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
>   {
> -	unsigned long flags, pending, issued;
> +	unsigned long flags;
> +	unsigned long pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> +	unsigned long issued = hba->outstanding_tasks & ~pending;
>   	irqreturn_t ret = IRQ_NONE;
>   	int tag;
>   
>   	spin_lock_irqsave(hba->host->host_lock, flags);
> -	pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> -	issued = hba->outstanding_tasks & ~pending;

Please keep the 'pending' and 'issued' assignments in the function body. 
Initializing variables in the declaration block is fine but adding code 
in the declaration block that has side effects is a bit controversial.

>   	for_each_set_bit(tag, &issued, hba->nutmrs) {
>   		struct request *req = hba->tmf_rqs[tag];
>   		struct completion *c = req->end_io_data;

Would it be sufficient to hold the SCSI host lock around the 
hba->outstanding_tasks read only? I don't think that the
for_each_set_bit() loop needs to be protected with the SCSI host lock.

Otherwise this patch looks good to me.

Thanks,

Bart.
Avri Altman Oct. 23, 2024, 6:47 a.m. UTC | #2 
> On 10/22/24 12:43 AM, Avri Altman wrote:
> > @@ -6877,13 +6874,13 @@ static irqreturn_t ufshcd_check_errors(struct
> ufs_hba *hba, u32 intr_status)
> >    */
> >   static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
> >   {
> > -     unsigned long flags, pending, issued;
> > +     unsigned long flags;
> > +     unsigned long pending = ufshcd_readl(hba,
> REG_UTP_TASK_REQ_DOOR_BELL);
> > +     unsigned long issued = hba->outstanding_tasks & ~pending;
> >       irqreturn_t ret = IRQ_NONE;
> >       int tag;
> >
> >       spin_lock_irqsave(hba->host->host_lock, flags);
> > -     pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> > -     issued = hba->outstanding_tasks & ~pending;
> 
> Please keep the 'pending' and 'issued' assignments in the function body.
> Initializing variables in the declaration block is fine but adding code in the
> declaration block that has side effects is a bit controversial.
Done.

> 
> >       for_each_set_bit(tag, &issued, hba->nutmrs) {
> >               struct request *req = hba->tmf_rqs[tag];
> >               struct completion *c = req->end_io_data;
> 
> Would it be sufficient to hold the SCSI host lock around the
> hba->outstanding_tasks read only? I don't think that the
> for_each_set_bit() loop needs to be protected with the SCSI host lock.
That may cause concurrent access to tmf_rqs?
So better withdraw from changing ufshcd_tmc_handler() and just leave the whole function as it is?

Thanks,
Avri
> 
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.
Bart Van Assche Oct. 23, 2024, 7:44 p.m. UTC | #3 
On 10/22/24 11:47 PM, Avri Altman wrote:
>> On 10/22/24 12:43 AM, Avri Altman wrote:
>>>        for_each_set_bit(tag, &issued, hba->nutmrs) {
>>>                struct request *req = hba->tmf_rqs[tag];
>>>                struct completion *c = req->end_io_data;
>>
>> Would it be sufficient to hold the SCSI host lock around the
>> hba->outstanding_tasks read only? I don't think that the
>> for_each_set_bit() loop needs to be protected with the SCSI host lock.
 >
> That may cause concurrent access to tmf_rqs?

Right, the host_lock serializes hba->tmf_rqs[] accesses. Without having
analyzed whether or not removing locking from around the hba->tmf_rqs[]
accesses, let's keep this locking.

> So better withdraw from changing ufshcd_tmc_handler() and just leave
> the whole function as it is?
That sounds good to me.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9e6d008f4ea4..29f1cd3375bd 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;
 }
@@ -6877,13 +6874,13 @@  static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba, u32 intr_status)
  */
 static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
 {
-	unsigned long flags, pending, issued;
+	unsigned long flags;
+	unsigned long pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
+	unsigned long issued = hba->outstanding_tasks & ~pending;
 	irqreturn_t ret = IRQ_NONE;
 	int tag;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
-	issued = hba->outstanding_tasks & ~pending;
 	for_each_set_bit(tag, &issued, hba->nutmrs) {
 		struct request *req = hba->tmf_rqs[tag];
 		struct completion *c = req->end_io_data;
@@ -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 */