On systems with a large number request slots and unavailable MCQ ESI,
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 wake up the threaded interrupt routine,
the interrupt line would be masked until the processing is
finished in the thread thanks to the IRQS_ONESHOT flag.
When MCQ & ESI interrupts are enabled the I/O completions are now
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)
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/ufs/core/ufshcd.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7f256f77b8ba9853569157db7785d177b6cd6dee..b40660ca2fa6b3488645bd26121752554a8d6a08 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,29 @@ 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 & ESI are not enabled */
+ if (!hba->mcq_enabled || !hba->mcq_esi_enabled)
+ return IRQ_WAKE_THREAD;
+
+ /* Directly handle interrupts since MCQ ESI 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;
@@ -10577,7 +10600,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
On 4/7/2025 3:47 PM, Neil Armstrong wrote:
> On systems with a large number request slots and unavailable MCQ ESI,
> 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 wake up the threaded interrupt routine,
> the interrupt line would be masked until the processing is
> finished in the thread thanks to the IRQS_ONESHOT flag.
>
> When MCQ & ESI interrupts are enabled the I/O completions are now
> 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.
Hi Neil,
I tested this change on both SM8750 and SM8650 using the upstream kernel
with MCQ enabled locally. I also validated it on the SM8750 downstream
codebase. In all cases, enabling MCQ mode led to boot-up issues on these
targets.
The root cause was that in MCQ mode, the Interrupt Status (IS) register
was not being cleared in the ufshcd_intr function. This resulted in
abnormal behavior during subsequent UIC commands, ultimately causing
boot failures.
To address this issue, I’ve submitted the following patch: [PATCH V1]
ufs: core: Fix interrupt handling for MCQ Mode in ufshcd_intr.
I also have plan to get the performance number with SDB and MCQ mode
with these change. I'll update the thread once i get the number.
Thanks,
Nitin
>
> 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>
> ---
> drivers/ufs/core/ufshcd.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 7f256f77b8ba9853569157db7785d177b6cd6dee..b40660ca2fa6b3488645bd26121752554a8d6a08 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,29 @@ 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 & ESI are not enabled */
> + if (!hba->mcq_enabled || !hba->mcq_esi_enabled)
> + return IRQ_WAKE_THREAD;
> +
> + /* Directly handle interrupts since MCQ ESI 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;
> @@ -10577,7 +10600,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;
>
Hi,
On Mon, 2025-04-07 at 12:17 +0200, Neil Armstrong wrote:
> On systems with a large number request slots and unavailable MCQ ESI,
> 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 wake up the threaded interrupt routine,
> the interrupt line would be masked until the processing is
> finished in the thread thanks to the IRQS_ONESHOT flag.
>
> When MCQ & ESI interrupts are enabled the I/O completions are now
> directly handled in the "hard" interrupt routine to keep IOPs high
> since queues handling is done in separate per-queue interrupt routines.
This patch adversely affects Pixel 6 UFS performance. It has a
UFSHCI v3.x controller I believe (and therefore probably all
devices with < v4) - if my limited understanding is correct,
MCQ & ESI are a feature of v4 controllers only.
On Pixel 6, fio reports following performance on linux-next with
this patch:
read [1] / write [2]:
READ: bw=17.1MiB/s (17.9MB/s), 17.1MiB/s-17.1MiB/s (17.9MB/s-17.9MB/s), io=684MiB (718MB), run=40001-40001msec
WRITE: bw=20.6MiB/s (21.5MB/s), 20.6MiB/s-20.6MiB/s (21.5MB/s-21.5MB/s), io=822MiB (862MB), run=40003-40003msec
With this patch reverted, performance changes back to:
read [1] / write [2]:
READ: bw=19.9MiB/s (20.8MB/s), 19.9MiB/s-19.9MiB/s (20.8MB/s-20.8MB/s), io=795MiB (833MB), run=40001-40001msec
WRITE: bw=28.0MiB/s (29.4MB/s), 28.0MiB/s-28.0MiB/s (29.4MB/s-29.4MB/s), io=1122MiB (1176MB), run=40003-40003msec
all over multiple runs.
which is a ~26% reduction for write and ~14% reduction for read.
PCBenchmark even reports performance drops of ~41%.
I don't know much about UFS at this stage, but could the code simply
check for the controller version and revert to original behaviour
if < v4? Any thoughts on such a change?
[1]: fio --name=randread --rw=randread --ioengine=libaio --direct=1 \
--bs=4k --numjobs=1 --size=1g --ramp_time=10 --runtime=40 --time_based \
--end_fsync=1 --group_reporting --filename=/foo
[2]: fio --name=randwrite --rw=randwrite --ioengine=libaio --direct=1 \
--bs=4k --numjobs=1 --size=1g --ramp_time=10 --runtime=40 --time_based \
--end_fsync=1 --group_reporting --filename=/foo
Cheers,
Andre'
On 7/21/25 5:04 AM, André Draszik wrote: > I don't know much about UFS at this stage, but could the code simply > check for the controller version and revert to original behaviour > if < v4? Any thoughts on such a change? I'm not sure that's the best possible solution. A more elegant solution could be to remove the "if (!hba->mcq_enabled || !hba->mcq_esi_enabled)" check, to restrict the number of commands completed by ufshcd_transfer_req_compl() and only to return IRQ_WAKE_THREAD if more commands are pending than a certain threshold. Thanks, Bart.
On Mon, 2025-07-21 at 08:28 -0700, Bart Van Assche wrote: > On 7/21/25 5:04 AM, André Draszik wrote: > > I don't know much about UFS at this stage, but could the code simply > > check for the controller version and revert to original behaviour > > if < v4? Any thoughts on such a change? > > I'm not sure that's the best possible solution. A more elegant solution > could be to remove the "if (!hba->mcq_enabled || !hba->mcq_esi_enabled)" > check, to restrict the number of commands completed by > ufshcd_transfer_req_compl() and only to return IRQ_WAKE_THREAD if more > commands are pending than a certain threshold. Thanks Bart. I'll try that instead, Cheers, Andre'
On Tue, 2025-07-22 at 10:22 +0100, André Draszik wrote: > On Mon, 2025-07-21 at 08:28 -0700, Bart Van Assche wrote: > > On 7/21/25 5:04 AM, André Draszik wrote: > > > I don't know much about UFS at this stage, but could the code simply > > > check for the controller version and revert to original behaviour > > > if < v4? Any thoughts on such a change? > > > > I'm not sure that's the best possible solution. A more elegant solution > > could be to remove the "if (!hba->mcq_enabled || !hba->mcq_esi_enabled)" > > check, to restrict the number of commands completed by > > ufshcd_transfer_req_compl() and only to return IRQ_WAKE_THREAD if more > > commands are pending than a certain threshold. > > Thanks Bart. I'll try that instead, I went with a time limit instead of counting the requests in the end, because that should be more deterministic: https://lore.kernel.org/r/20250724-ufshcd-hardirq-v1-1-6398a52f8f02@linaro.org Cheers, Andre'
On Mon, 2025-07-21 at 13:04 +0100, André Draszik wrote: > Hi, > > On Mon, 2025-04-07 at 12:17 +0200, Neil Armstrong wrote: > > On systems with a large number request slots and unavailable MCQ ESI, > > 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 wake up the threaded interrupt routine, > > the interrupt line would be masked until the processing is > > finished in the thread thanks to the IRQS_ONESHOT flag. > > > > When MCQ & ESI interrupts are enabled the I/O completions are now > > directly handled in the "hard" interrupt routine to keep IOPs high > > since queues handling is done in separate per-queue interrupt routines. > > This patch adversely affects Pixel 6 UFS performance. It has a > UFSHCI v3.x controller I believe (and therefore probably all > devices with < v4) - if my limited understanding is correct, > MCQ & ESI are a feature of v4 controllers only. > > On Pixel 6, fio reports following performance on linux-next with > this patch: > > read [1] / write [2]: > READ: bw=17.1MiB/s (17.9MB/s), 17.1MiB/s-17.1MiB/s (17.9MB/s-17.9MB/s), io=684MiB (718MB), run=40001-40001msec > WRITE: bw=20.6MiB/s (21.5MB/s), 20.6MiB/s-20.6MiB/s (21.5MB/s-21.5MB/s), io=822MiB (862MB), run=40003-40003msec > > With this patch reverted, performance changes back to: > > read [1] / write [2]: > > READ: bw=19.9MiB/s (20.8MB/s), 19.9MiB/s-19.9MiB/s (20.8MB/s-20.8MB/s), io=795MiB (833MB), run=40001-40001msec > WRITE: bw=28.0MiB/s (29.4MB/s), 28.0MiB/s-28.0MiB/s (29.4MB/s-29.4MB/s), io=1122MiB (1176MB), run=40003-40003msec > > all over multiple runs. > > which is a ~26% reduction for write and ~14% reduction for read. > > PCBenchmark even reports performance drops of ~41%. Additional fio results (numjobs=8 instead of 1): current linux-next: fio --name=randread --rw=randread --ioengine=libaio --direct=1 --bs=4k --numjobs=8 --size=1g --runtime=30 --time_based --end_fsync=1 -- group_reporting --filename=/foo READ: bw=52.1MiB/s (54.6MB/s), 52.1MiB/s-52.1MiB/s (54.6MB/s-54.6MB/s), io=1562MiB (1638MB), run=30001-30001msec WRITE: bw=74.7MiB/s (78.3MB/s), 74.7MiB/s-74.7MiB/s (78.3MB/s-78.3MB/s), io=2242MiB (2351MB), run=30004-30004msec with patch reverted: fio --name=randread --rw=randread --ioengine=libaio --direct=1 --bs=4k --numjobs=8 --size=1g --runtime=30 --time_based --end_fsync=1 -- group_reporting --filename=/foo READ: bw=83.5MiB/s (87.6MB/s), 83.5MiB/s-83.5MiB/s (87.6MB/s-87.6MB/s), io=2506MiB (2628MB), run=30001-30001msec WRITE: bw=83.3MiB/s (87.4MB/s), 83.3MiB/s-83.3MiB/s (87.4MB/s-87.4MB/s), io=2501MiB (2622MB), run=30003-30003msec which is an even higher 37% reduction for read. A.
On 4/7/25 3:17 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 & ESI are not enabled */
> + if (!hba->mcq_enabled || !hba->mcq_esi_enabled)
> + return IRQ_WAKE_THREAD;
> +
> + /* Directly handle interrupts since MCQ ESI handlers does the hard job */
> + return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) &
> + ufshcd_readl(hba, REG_INTERRUPT_ENABLE));
> +}
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
© 2016 - 2025 Red Hat, Inc.