Re: [PATCH] ufs: core: Revert "Delegate the interrupt service routine to a threaded IRQ handler"

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello Bart,

> There have been multiple reports of performance regressions caused by
> commit 3c7ac40d7322. Hence this revert.
>
> This patch reverts most of the following commits:
> * 3c7ac40d7322 ("scsi: ufs: core: Delegate the interrupt service routine to
>   a threaded IRQ handler")
> * 6475cfb81fc4 ("scsi: ufs: core: Avoid IRQ thread wakeup during active UIC
>   command")
>
> This patch preserves the following commits:
> * 034d319c8899 ("scsi: ufs: core: Fix interrupt handling for MCQ Mode")
> * eabcac808ca3 ("scsi: ufs: core: Fix IRQ lock inversion for the SCSI host
>   lock")
>
> Neil, please check whether this patch series fixes the stalls that you
> reported: [f2fs-dev] [PATCH 0/4] Reduce the time spent in interrupt context
> (https://sourceforge.net/p/linux-f2fs/mailman/linux-f2fs-devel/thread/4b337c7f-2fbd-47b6-9c8c-587e7b5513f1%40kernel.org/#msg59345049).
>

This patch appears to be a good solution to fix the issue I reported
with PREEMPT_TR enabled. I don't currently have the hardware available
to test it, but as soon as I do, I will test this patch.
Thanks,

Gregory


> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: 孙魁 (Kui Sun) <kui.sun@unisoc.com>
> Cc: André Draszik <andre.draszik@linaro.org>
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Fixes: 3c7ac40d7322 ("scsi: ufs: core: Delegate the interrupt service routine to a threaded IRQ handler")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 39 +++------------------------------------
>  1 file changed, 3 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 79331c2bd38d..56c96a3b10e2 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7356,7 +7356,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>  }
>  
>  /**
> - * ufshcd_threaded_intr - Threaded interrupt service routine
> + * ufshcd_intr - Main interrupt service routine
>   * @irq: irq number
>   * @__hba: pointer to adapter instance
>   *
> @@ -7364,7 +7364,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_threaded_intr(int irq, void *__hba)
> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
>  {
>  	u32 last_intr_status, intr_status, enabled_intr_status = 0;
>  	irqreturn_t retval = IRQ_NONE;
> @@ -7403,38 +7403,6 @@ static irqreturn_t ufshcd_threaded_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;
> -	u32 intr_status, enabled_intr_status;
> -
> -	/*
> -	 * Handle interrupt in thread if MCQ or ESI is disabled,
> -	 * and no active UIC command.
> -	 */
> -	if ((!hba->mcq_enabled || !hba->mcq_esi_enabled) &&
> -	    !hba->active_uic_cmd)
> -		return IRQ_WAKE_THREAD;
> -
> -	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> -	enabled_intr_status = intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> -
> -	ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> -
> -	/* Directly handle interrupts since MCQ ESI handlers does the hard job */
> -	return ufshcd_sl_intr(hba, enabled_intr_status);
> -}
> -
>  static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
>  {
>  	int err = 0;
> @@ -11233,8 +11201,7 @@ 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_threaded_irq(dev, irq, ufshcd_intr, ufshcd_threaded_intr,
> -					IRQF_ONESHOT | IRQF_SHARED, UFSHCD, hba);
> +	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
>  	if (err) {
>  		dev_err(hba->dev, "request irq failed\n");
>  		goto out_disable;

-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com