scsi: ufs: make sure all interrupts are processed

PATCH In review Asutosh Das 2 participants 5 messages Jan 30, 2018 – Feb 5, 2018 SCSI

lore

Share & tools

lore

Asutosh Das

author

2018-01-30 04:54 UTC #0 lore raw

PATCH 1/1 scsi: ufs: make sure all interrupts are processed

From: Venkat Gopalakrishnan <[email protected]>

As multiple requests are submitted to the ufs host controller in parallel there could be instances where the command completion interrupt arrives later for a request that is already processed earlier as the corresponding doorbell was cleared when handling the previous interrupt. Read the interrupt status in a loop after processing the received interrupt to catch such interrupts and handle it.

Signed-off-by Venkat Gopalakrishnan <[email protected]>

Signed-off-by Asutosh Das <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufshcd.c +19 -8

index 8af2af3..58d81de 100644
@@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 	u32 intr_status, enabled_intr_status;
 	irqreturn_t retval = IRQ_NONE;
 	struct ufs_hba *hba = __hba;
+	int retries = hba->nutrs;
 
 	spin_lock(hba->host->host_lock);
 	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-	enabled_intr_status =
-		intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (intr_status)
-		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+	/*
+	 * There could be max of hba->nutrs reqs in flight and in worst case
+	 * if the reqs get finished 1 by 1 after the interrupt status is
+	 * read, make sure we handle them by checking the interrupt status
+	 * again in a loop until we process all of the reqs before returning.
+	 */
+	do {
+		enabled_intr_status =
+			intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
+		if (intr_status)
+			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+		if (enabled_intr_status) {
+			ufshcd_sl_intr(hba, enabled_intr_status);
+			retval = IRQ_HANDLED;
+		}
+
+		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
+	} while (intr_status && --retries);
 
-	if (enabled_intr_status) {
-		ufshcd_sl_intr(hba, enabled_intr_status);
-		retval = IRQ_HANDLED;
-	}
 	spin_unlock(hba->host->host_lock);
 	return retval;
 }

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Avri Altman

2018-01-31 07:39 UTC #1 lore raw

Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?

Thanks,
Avri

Quoted 78 lines

-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Venkat Gopalakrishnan <[email protected]>; Asutosh Das <[email protected]>; open list <[email protected]>
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
From: Venkat Gopalakrishnan <[email protected]>
As multiple requests are submitted to the ufs host controller in parallel there could be instances where the command completion interrupt arrives later for a request that is already processed earlier as the corresponding doorbell was cleared when handling the previous interrupt. Read the interrupt status in a loop after processing the received interrupt to catch such interrupts and handle it.
Signed-off-by Venkat Gopalakrishnan <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
drivers/scsi/ufs/ufshcd.c index renamed from drivers/scsi/ufs/ufshcd.c +0 -0
8af2af3..58d81de 100644
drivers/scsi/ufs/ufshcd.c +19 -8
@@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 	u32 intr_status, enabled_intr_status;
 	irqreturn_t retval = IRQ_NONE;
 	struct ufs_hba *hba = __hba;
+	int retries = hba->nutrs;
 
 	spin_lock(hba->host->host_lock);
 	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-	enabled_intr_status =
-		intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (intr_status)
-		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+	/*
+	 * There could be max of hba->nutrs reqs in flight and in worst case
+	 * if the reqs get finished 1 by 1 after the interrupt status is
+	 * read, make sure we handle them by checking the interrupt status
+	 * again in a loop until we process all of the reqs before returning.
+	 */
+	do {
+		enabled_intr_status =
+			intr_status & ufshcd_readl(hba,
 REG_INTERRUPT_ENABLE);
+		if (intr_status)
+			ufshcd_writel(hba, intr_status,
 REG_INTERRUPT_STATUS);
+		if (enabled_intr_status) {
+			ufshcd_sl_intr(hba, enabled_intr_status);
+			retval = IRQ_HANDLED;
+		}
+
+		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
+	} while (intr_status && --retries);
 
-	if (enabled_intr_status) {
-		ufshcd_sl_intr(hba, enabled_intr_status);
-		retval = IRQ_HANDLED;
-	}
 	spin_unlock(hba->host->host_lock);
return retval;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Asutosh Das (asd)

author

2018-02-02 03:23 UTC #2 lore raw

Quoted 73 lines · Avri Altman

Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?
Thanks,
Avri
Quoted 66 lines
-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Venkat Gopalakrishnan <[email protected]>; Asutosh Das <[email protected]>; open list <[email protected]>
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
From: Venkat Gopalakrishnan <[email protected]>
As multiple requests are submitted to the ufs host controller in parallel there could be instances where the command completion interrupt arrives later for a request that is already processed earlier as the corresponding doorbell was cleared when handling the previous interrupt. Read the interrupt status in a loop after processing the received interrupt to catch such interrupts and handle it.
Signed-off-by Venkat Gopalakrishnan <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
  drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)
drivers/scsi/ufs/ufshcd.c index renamed from drivers/scsi/ufs/ufshcd.c +0 -0
8af2af3..58d81de 100644
drivers/scsi/ufs/ufshcd.c +19 -4
@@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
  	u32 intr_status, enabled_intr_status;
  	irqreturn_t retval = IRQ_NONE;
  	struct ufs_hba *hba = __hba;
+	int retries = hba->nutrs;
 
  	spin_lock(hba->host->host_lock);
  	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-	enabled_intr_status =
-		intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (intr_status)
-		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+	/*
+	 * There could be max of hba->nutrs reqs in flight and in worst case
+	 * if the reqs get finished 1 by 1 after the interrupt status is
+	 * read, make sure we handle them by checking the interrupt status
+	 * again in a loop until we process all of the reqs before returning.
+	 */
+	do {
+		enabled_intr_status =
+			intr_status & ufshcd_readl(hba,
 REG_INTERRUPT_ENABLE);
+		if (intr_status)
+			ufshcd_writel(hba, intr_status,
 REG_INTERRUPT_STATUS);
+		if (enabled_intr_status) {
+			ufshcd_sl_intr(hba, enabled_intr_status);
+			retval = IRQ_HANDLED;
+		}
+
+		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
+	} while (intr_status && --retries);
 

Quoted 13 lines

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +0 -4
-	if (enabled_intr_status) {
-		ufshcd_sl_intr(hba, enabled_intr_status);
-		retval = IRQ_HANDLED;
-	}
  	spin_unlock(hba->host->host_lock);
Quoted 7 lines
return retval;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Hi
yes - interrupt aggregation makes sense here. But there were some performance concerns with it; well, I don't have the data to back that up now though. However, I can code it up and check it.
Will post it in some time.

-asd

Signature · 1 line

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Asutosh Das (asd)

author

2018-02-05 04:57 UTC #3 lore raw

Quoted 91 lines · Asutosh Das (asd)

Quoted 76 lines · Avri Altman
Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?
Thanks,
Avri
Quoted 69 lines
-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Venkat Gopalakrishnan <[email protected]>; Asutosh Das <[email protected]>; open list <[email protected]>
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
From: Venkat Gopalakrishnan <[email protected]>
As multiple requests are submitted to the ufs host controller in parallel there could be instances where the command completion interrupt arrives later for a request that is already processed earlier as the corresponding doorbell was cleared when handling the previous interrupt. Read the interrupt status in a loop after processing the received interrupt to catch such interrupts and handle it.
Signed-off-by Venkat Gopalakrishnan <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)
drivers/scsi/ufs/ufshcd.c index renamed from drivers/scsi/ufs/ufshcd.c +0 -0
8af2af3..58d81de 100644
drivers/scsi/ufs/ufshcd.c +3 -4
@@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void 
 *__hba)
       u32 intr_status, enabled_intr_status;
       irqreturn_t retval = IRQ_NONE;
       struct ufs_hba *hba = __hba;
+    int retries = hba->nutrs;
 
       spin_lock(hba->host->host_lock);
       intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-    enabled_intr_status =
-        intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-    if (intr_status)
-        ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+    /*
+     * There could be max of hba->nutrs reqs in flight and in worst 
case
+     * if the reqs get finished 1 by 1 after the interrupt status is +     * read, make sure we handle them by checking the interrupt status +     * again in a loop until we process all of the reqs before returning.
+     */
+    do {
+        enabled_intr_status =
+            intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
+        if (intr_status)
+            ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+        if (enabled_intr_status) {
+            ufshcd_sl_intr(hba, enabled_intr_status);
+            retval = IRQ_HANDLED;
+        }
+
+        intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS); +    } while (intr_status && --retries);
Quoted 13 lines
Quoted 12 lines
-    if (enabled_intr_status) {
-        ufshcd_sl_intr(hba, enabled_intr_status); -        retval = IRQ_HANDLED;
-    }
      spin_unlock(hba->host->host_lock); return retval;
}
Signature · 4 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project.

Quoted 7 lines

Hi
yes - interrupt aggregation makes sense here. But there were some performance concerns with it; well, I don't have the data to back that up now though. However, I can code it up and check it.
Will post it in some time.
-asd

Hi Avri,
I went through the UFS HCI - v2.1 spec. Specifically, in sec 7.2.3 it explicitly mentions that the software should determine if new TRs were completed since the interrupt status was last read/cleared. This step is independent of aggregation.

So I think the above implementation makes sense. Please let me know if I understood your concern correctly.

-asd

Signature · 1 line

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Avri Altman

Tested-by

2018-02-05 12:57 UTC #4 lore raw

Quoted 26 lines

Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; Venkat Gopalakrishnan <[email protected]>; Asutosh Das <[email protected]>; open list <[email protected]>
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
From: Venkat Gopalakrishnan <[email protected]>
As multiple requests are submitted to the ufs host controller in parallel there could be instances where the command completion interrupt arrives later for a request that is already processed earlier as the corresponding doorbell was cleared when handling the previous interrupt. Read the interrupt status in a loop after processing the received interrupt to catch such interrupts and handle it.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Venkat Gopalakrishnan <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

Tested-by [email protected]

Tested on kirin960 (mate9) and msm8998 (htc11), both where interrupt aggregation is not allowed.

As a side note, I noticed that this patch is part of several patches, fixing some qcom-staff. Maybe you want to put them all in a patchset?

Thanks,
Avri