[PATCH RFC v2 0/2] ufs: core: cleanup and threaded irq handler

[PATCH RFC v2 0/2] ufs: core: cleanup and threaded irq handler

[Neil Armstrong]

On systems with a large number request slots and unavailable MCQ,
the current design of the interrupt handler can delay handling of
other subsystems interrupts causing display artifacts, GPU stalls
or system firmware requests timeouts.

Example of errors reported on a loaded system:
 [drm:dpu_encoder_frame_done_timeout:2706] [dpu error]enc32 frame done timeout
 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1: hangcheck detected gpu lockup rb 2!
 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1:     completed fence: 74285
 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1:     submitted fence: 74286
 Error sending AMC RPMH requests (-110)

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v2:
- Removed last_intr_status/last_intr_ts stats
- Handle irq in prinmary handler for MCQ case
- Stop touching REG_INTERRUPT_ENABLE register
- Link to v1: https://lore.kernel.org/r/20250321-topic-ufs-use-threaded-irq-v1-1-7a55816a4b1d@linaro.org

---
Neil Armstrong (2):
      ufs: core: drop last_intr_status/ts stats
      ufs: core: delegate the interrupt service routine to a threaded irq handler

 drivers/ufs/core/ufshcd.c | 45 ++++++++++++++++++++++++++++++++++-----------
 include/ufs/ufshcd.h      |  5 -----
 2 files changed, 34 insertions(+), 16 deletions(-)
---
base-commit: ff7f9b199e3f4cc7d61df5a9a26a7cbb5c1492e6
change-id: 20250321-topic-ufs-use-threaded-irq-53af30f2529f

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH RFC v2 1/2] ufs: core: drop last_intr_status/ts stats

[Neil Armstrong]

Drop last_intr_status & last_intr_ts drop the ufs_stats struct,
and the associated debug code.

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/ufs/core/ufshcd.c | 11 +++--------
 include/ufs/ufshcd.h      |  5 -----
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0534390c2a35d0671156d79a4b1981a257d2fbfa..5e73ac1e00788f3d599f0b3eb6e2806df9b6f6c3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -643,9 +643,6 @@ static void ufshcd_print_host_state(struct ufs_hba *hba)
 		"last_hibern8_exit_tstamp at %lld us, hibern8_exit_cnt=%d\n",
 		div_u64(hba->ufs_stats.last_hibern8_exit_tstamp, 1000),
 		hba->ufs_stats.hibern8_exit_cnt);
-	dev_err(hba->dev, "last intr at %lld us, last intr status=0x%x\n",
-		div_u64(hba->ufs_stats.last_intr_ts, 1000),
-		hba->ufs_stats.last_intr_status);
 	dev_err(hba->dev, "error handling flags=0x%x, req. abort count=%d\n",
 		hba->eh_flags, hba->req_abort_count);
 	dev_err(hba->dev, "hba->ufs_version=0x%x, Host capabilities=0x%x, caps=0x%x\n",
@@ -6984,14 +6981,12 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  */
 static irqreturn_t ufshcd_intr(int irq, void *__hba)
 {
-	u32 intr_status, enabled_intr_status = 0;
+	u32 last_intr_status, intr_status, enabled_intr_status = 0;
 	irqreturn_t retval = IRQ_NONE;
 	struct ufs_hba *hba = __hba;
 	int retries = hba->nutrs;
 
-	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-	hba->ufs_stats.last_intr_status = intr_status;
-	hba->ufs_stats.last_intr_ts = local_clock();
+	last_intr_status = intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 
 	/*
 	 * There could be max of hba->nutrs reqs in flight and in worst case
@@ -7015,7 +7010,7 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 		dev_err(hba->dev, "%s: Unhandled interrupt 0x%08x (0x%08x, 0x%08x)\n",
 					__func__,
 					intr_status,
-					hba->ufs_stats.last_intr_status,
+					last_intr_status,
 					enabled_intr_status);
 		ufshcd_dump_regs(hba, 0, UFSHCI_REG_SPACE_SIZE, "host_regs: ");
 	}
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index e3909cc691b2a854a270279901edacaa5c5120d6..fffa9cc465433604570f91b8e882b58cd985f35b 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -501,8 +501,6 @@ struct ufs_event_hist {
 
 /**
  * struct ufs_stats - keeps usage/err statistics
- * @last_intr_status: record the last interrupt status.
- * @last_intr_ts: record the last interrupt timestamp.
  * @hibern8_exit_cnt: Counter to keep track of number of exits,
  *		reset this after link-startup.
  * @last_hibern8_exit_tstamp: Set time after the hibern8 exit.
@@ -510,9 +508,6 @@ struct ufs_event_hist {
  * @event: array with event history.
  */
 struct ufs_stats {
-	u32 last_intr_status;
-	u64 last_intr_ts;
-
 	u32 hibern8_exit_cnt;
 	u64 last_hibern8_exit_tstamp;
 	struct ufs_event_hist event[UFS_EVT_CNT];

-- 
2.34.1

Re: [PATCH RFC v2 1/2] ufs: core: drop last_intr_status/ts stats

[Bart Van Assche]

On 3/26/25 4:36 AM, Neil Armstrong wrote:
> Drop last_intr_status & last_intr_ts drop the ufs_stats struct,
> and the associated debug code.

Patch descriptions should not only explain what has been changed but
also why a change is being made. In this case, this change prepares for
making an interrupt handler threaded. If this patch series has to be 
resent, please add this information to the patch description. Anyway,
since the patch itself looks good to me:

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

Re: [PATCH RFC v2 1/2] ufs: core: drop last_intr_status/ts stats

[Neil Armstrong]

On 27/03/2025 12:40, Bart Van Assche wrote:
> On 3/26/25 4:36 AM, Neil Armstrong wrote:
>> Drop last_intr_status & last_intr_ts drop the ufs_stats struct,
>> and the associated debug code.
> 
> Patch descriptions should not only explain what has been changed but
> also why a change is being made. In this case, this change prepares for
> making an interrupt handler threaded. If this patch series has to be resent, please add this information to the patch description. Anyway,
> since the patch itself looks good to me:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 

Ack will update the commit msg

Thanks,
Neil

[PATCH RFC v2 2/2] ufs: core: delegate the interrupt service routine to a threaded irq handler

[Neil Armstrong]

On systems with a large number request slots and unavailable MCQ,
the current design of the interrupt handler can delay handling of
other subsystems interrupts causing display artifacts, GPU stalls
or system firmware requests timeouts.

Since the interrupt routine can take quite some time, it's
preferable to move it to a threaded handler and leave the
hard interrupt handler save the status and disable the irq
until processing is finished in the thread.

When MCQ & Interrupt Aggregation are supported, the interrupt
are directly handled in the "hard" interrupt routine to
keep IOPs high since queues handling is done in separate
per-queue interrupt routines.

This fixes all encountered issued when running FIO tests
on the Qualcomm SM8650 platform.

Example of errors reported on a loaded system:
 [drm:dpu_encoder_frame_done_timeout:2706] [dpu error]enc32 frame done timeout
 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1: hangcheck detected gpu lockup rb 2!
 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1:     completed fence: 74285
 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1:     submitted fence: 74286
 Error sending AMC RPMH requests (-110)

Reported bandwidth is not affected on various tests.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/ufs/core/ufshcd.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5e73ac1e00788f3d599f0b3eb6e2806df9b6f6c3..5de25fc978dd7c4c1ac3b9ccbca2ab3f13d6aa65 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6971,7 +6971,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 }
 
 /**
- * ufshcd_intr - Main interrupt service routine
+ * ufshcd_threaded_intr - Threaded interrupt service routine
  * @irq: irq number
  * @__hba: pointer to adapter instance
  *
@@ -6979,7 +6979,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  *  IRQ_HANDLED - If interrupt is valid
  *  IRQ_NONE    - If invalid interrupt
  */
-static irqreturn_t ufshcd_intr(int irq, void *__hba)
+static irqreturn_t ufshcd_threaded_intr(int irq, void *__hba)
 {
 	u32 last_intr_status, intr_status, enabled_intr_status = 0;
 	irqreturn_t retval = IRQ_NONE;
@@ -7018,6 +7018,33 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 	return retval;
 }
 
+/**
+ * ufshcd_intr - Main interrupt service routine
+ * @irq: irq number
+ * @__hba: pointer to adapter instance
+ *
+ * Return:
+ *  IRQ_HANDLED     - If interrupt is valid
+ *  IRQ_WAKE_THREAD - If handling is moved to threaded handled
+ *  IRQ_NONE        - If invalid interrupt
+ */
+static irqreturn_t ufshcd_intr(int irq, void *__hba)
+{
+	struct ufs_hba *hba = __hba;
+
+	/*
+	 * Move interrupt handling to thread when MCQ is not supported
+	 * or when Interrupt Aggregation is not supported, leading to
+	 * potentially longer interrupt handling.
+	 */
+	if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba))
+		return IRQ_WAKE_THREAD;
+
+	/* Directly handle interrupts since MCQ handlers does the hard job */
+	return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) &
+				   ufshcd_readl(hba, REG_INTERRUPT_ENABLE));
+}
+
 static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
 {
 	int err = 0;
@@ -10576,7 +10603,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
 	/* IRQ registration */
-	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
+	err = devm_request_threaded_irq(dev, irq, ufshcd_intr, ufshcd_threaded_intr,
+					IRQF_ONESHOT | IRQF_SHARED, UFSHCD, hba);
 	if (err) {
 		dev_err(hba->dev, "request irq failed\n");
 		goto out_disable;

-- 
2.34.1

Re: [PATCH RFC v2 2/2] ufs: core: delegate the interrupt service routine to a threaded irq handler

[Bart Van Assche]

On 3/26/25 4:36 AM, Neil Armstrong wrote:
 > When MCQ & Interrupt Aggregation are supported, the interrupt
 > are directly handled in the "hard" interrupt routine to
 > keep IOPs high since queues handling is done in separate
 > per-queue interrupt routines.

The above explanation suggests that I/O completions are handled by the
modified interrupt handler. This is not necessarily the case. With MCQ,
I/O completions are either handled by dedicated interrupts or by the
legacy interrupt handler.

> Reported bandwidth is not affected on various tests.

This kind of patch can only affect command completion latency but not
the bandwidth, isn't it?

> +/**
> + * ufshcd_intr - Main interrupt service routine
> + * @irq: irq number
> + * @__hba: pointer to adapter instance
> + *
> + * Return:
> + *  IRQ_HANDLED     - If interrupt is valid
> + *  IRQ_WAKE_THREAD - If handling is moved to threaded handled
> + *  IRQ_NONE        - If invalid interrupt
> + */
> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
> +{
> +	struct ufs_hba *hba = __hba;
> +
> +	/*
> +	 * Move interrupt handling to thread when MCQ is not supported
> +	 * or when Interrupt Aggregation is not supported, leading to
> +	 * potentially longer interrupt handling.
> +	 */
> +	if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba))
> +		return IRQ_WAKE_THREAD;
> +
> +	/* Directly handle interrupts since MCQ handlers does the hard job */
> +	return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) &
> +				   ufshcd_readl(hba, REG_INTERRUPT_ENABLE));
> +}

Where has ufshcd_is_intr_aggr_allowed() been defined? I can't find this
function.

For the MCQ case, this patch removes the loop from around
ufshcd_sl_intr() without explaining in the patch description why this 
change has been made. Please explain all changes in the patch
description.

Thanks,

Bart.

Re: [PATCH RFC v2 2/2] ufs: core: delegate the interrupt service routine to a threaded irq handler

[Neil Armstrong]

Hi,

On 27/03/2025 12:56, Bart Van Assche wrote:
> On 3/26/25 4:36 AM, Neil Armstrong wrote:
>  > When MCQ & Interrupt Aggregation are supported, the interrupt
>  > are directly handled in the "hard" interrupt routine to
>  > keep IOPs high since queues handling is done in separate
>  > per-queue interrupt routines.
> 
> The above explanation suggests that I/O completions are handled by the
> modified interrupt handler. This is not necessarily the case. With MCQ,
> I/O completions are either handled by dedicated interrupts or by the
> legacy interrupt handler.

Will update the sentence with that

> 
>> Reported bandwidth is not affected on various tests.
> 
> This kind of patch can only affect command completion latency but not
> the bandwidth, isn't it?

Yes, but on a fully loaded system, it will enhance bandwidth
but with a greater latency, but without eating irq handling time
for other routines.

> 
>> +/**
>> + * ufshcd_intr - Main interrupt service routine
>> + * @irq: irq number
>> + * @__hba: pointer to adapter instance
>> + *
>> + * Return:
>> + *  IRQ_HANDLED     - If interrupt is valid
>> + *  IRQ_WAKE_THREAD - If handling is moved to threaded handled
>> + *  IRQ_NONE        - If invalid interrupt
>> + */
>> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
>> +{
>> +    struct ufs_hba *hba = __hba;
>> +
>> +    /*
>> +     * Move interrupt handling to thread when MCQ is not supported
>> +     * or when Interrupt Aggregation is not supported, leading to
>> +     * potentially longer interrupt handling.
>> +     */
>> +    if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba))
>> +        return IRQ_WAKE_THREAD;
>> +
>> +    /* Directly handle interrupts since MCQ handlers does the hard job */
>> +    return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) &
>> +                   ufshcd_readl(hba, REG_INTERRUPT_ENABLE));
>> +}
> 
> Where has ufshcd_is_intr_aggr_allowed() been defined? I can't find this
> function.

It's in include/ufs/ufshcd.h

> 
> For the MCQ case, this patch removes the loop from around
> ufshcd_sl_intr() without explaining in the patch description why this change has been made. Please explain all changes in the patch
> description.

Ack will update explaining this change.

Thanks,
Neil

> 
> Thanks,
> 
> Bart.

Re: [PATCH RFC v2 2/2] ufs: core: delegate the interrupt service routine to a threaded irq handler

[Bart Van Assche]

On 3/26/25 1:36 AM, Neil Armstrong wrote:
> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
> +{
> +	struct ufs_hba *hba = __hba;
> +
> +	/*
> +	 * Move interrupt handling to thread when MCQ is not supported
> +	 * or when Interrupt Aggregation is not supported, leading to
> +	 * potentially longer interrupt handling.
> +	 */
> +	if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba))
> +		return IRQ_WAKE_THREAD;
> +
> +	/* Directly handle interrupts since MCQ handlers does the hard job */
> +	return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) &
> +				   ufshcd_readl(hba, REG_INTERRUPT_ENABLE));
> +}

Calling ufshcd_is_intr_aggr_allowed() from the above interrupt handler
seems wrong to me. I think you want to check whether or not ESI has been
disabled since only if ESI is disabled all I/O completions are handled
by a single interrupt if MCQ is enabled.

Thanks,

Bart.