Re: [PATCH V2] ufs: ufs-qcom: Fix ESI null pointer dereference

[Nitin Rawat <quic_nitirawa@quicinc.com>]

On 8/11/2025 1:03 PM, Nitin Rawat wrote:
> ESI/MSI is a performance optimization feature that provides dedicated
> interrupts per MCQ hardware queue . This is optional feature and
> UFS MCQ should work with and without ESI feature.
> 
> Commit e46a28cea29a ("scsi: ufs: qcom: Remove the MSI descriptor abuse")
> brings a regression in ESI (Enhanced System Interrupt) configuration
> that causes a null pointer dereference when Platform MSI allocation
> fails.
> 
> The issue occurs in when platform_device_msi_init_and_alloc_irqs()
> in ufs_qcom_config_esi() fails (returns -EINVAL) but the current
> code uses __free() macro for automatic cleanup free MSI resources
> that were never successfully allocated.
> 
> Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000008
> 
>    Call trace:
>    mutex_lock+0xc/0x54 (P)
>    platform_device_msi_free_irqs_all+0x1c/0x40
>    ufs_qcom_config_esi+0x1d0/0x220 [ufs_qcom]
>    ufshcd_config_mcq+0x28/0x104
>    ufshcd_init+0xa3c/0xf40
>    ufshcd_pltfrm_init+0x504/0x7d4
>    ufs_qcom_probe+0x20/0x58 [ufs_qcom]
> 
> Fix by restructuring the ESI configuration to try MSI allocation
> first, before any other resource allocation and instead use
> explicit cleanup instead of __free() macro to avoid cleanup
> of unallocated resources.
> 
> Tested on SM8750 platform with MCQ enabled, both with and without
> Platform ESI support.
> 
> Fixes: e46a28cea29a ("scsi: ufs: qcom: Remove the MSI descriptor abuse")
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
> Changes from v1:
> 1. Added correct sha1 of change id which caused regression.
> 2. Address Markus comment to add fixes: and Cc: tags.
> ---
>   drivers/ufs/host/ufs-qcom.c | 39 ++++++++++++++-----------------------
>   1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 4bbe4de1679b..bef8dc12de20 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -2078,17 +2078,6 @@ static irqreturn_t ufs_qcom_mcq_esi_handler(int irq, void *data)
>   	return IRQ_HANDLED;
>   }
> 
> -static void ufs_qcom_irq_free(struct ufs_qcom_irq *uqi)
> -{
> -	for (struct ufs_qcom_irq *q = uqi; q->irq; q++)
> -		devm_free_irq(q->hba->dev, q->irq, q->hba);
> -
> -	platform_device_msi_free_irqs_all(uqi->hba->dev);
> -	devm_kfree(uqi->hba->dev, uqi);
> -}
> -
> -DEFINE_FREE(ufs_qcom_irq, struct ufs_qcom_irq *, if (_T) ufs_qcom_irq_free(_T))
> -
>   static int ufs_qcom_config_esi(struct ufs_hba *hba)
>   {
>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> @@ -2103,18 +2092,18 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>   	 */
>   	nr_irqs = hba->nr_hw_queues - hba->nr_queues[HCTX_TYPE_POLL];
> 
> -	struct ufs_qcom_irq *qi __free(ufs_qcom_irq) =
> -		devm_kcalloc(hba->dev, nr_irqs, sizeof(*qi), GFP_KERNEL);
> -	if (!qi)
> -		return -ENOMEM;
> -	/* Preset so __free() has a pointer to hba in all error paths */
> -	qi[0].hba = hba;
> -
>   	ret = platform_device_msi_init_and_alloc_irqs(hba->dev, nr_irqs,
>   						      ufs_qcom_write_msi_msg);
>   	if (ret) {
> -		dev_err(hba->dev, "Failed to request Platform MSI %d\n", ret);
> -		return ret;
> +		dev_warn(hba->dev, "Platform MSI not supported or failed, continuing without ESI\n");
> +		return ret; /* Continue without ESI */
> +	}
> +
> +	struct ufs_qcom_irq *qi = devm_kcalloc(hba->dev, nr_irqs, sizeof(*qi), GFP_KERNEL);
> +
> +	if (!qi) {
> +		platform_device_msi_free_irqs_all(hba->dev);
> +		return -ENOMEM;
>   	}
> 
>   	for (int idx = 0; idx < nr_irqs; idx++) {
> @@ -2125,15 +2114,17 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>   		ret = devm_request_irq(hba->dev, qi[idx].irq, ufs_qcom_mcq_esi_handler,
>   				       IRQF_SHARED, "qcom-mcq-esi", qi + idx);
>   		if (ret) {
> -			dev_err(hba->dev, "%s: Fail to request IRQ for %d, err = %d\n",
> +			dev_err(hba->dev, "%s: Failed to request IRQ for %d, err = %d\n",
>   				__func__, qi[idx].irq, ret);
> -			qi[idx].irq = 0;
> +			/* Free previously allocated IRQs */
> +			for (int j = 0; j < idx; j++)
> +				devm_free_irq(hba->dev, qi[j].irq, qi + j);
> +			platform_device_msi_free_irqs_all(hba->dev);
> +			devm_kfree(hba->dev, qi);
>   			return ret;
>   		}
>   	}
> 
> -	retain_and_null_ptr(qi);
> -
>   	if (host->hw_ver.major >= 6) {
>   		ufshcd_rmwl(hba, ESI_VEC_MASK, FIELD_PREP(ESI_VEC_MASK, MAX_ESI_VEC - 1),
>   			    REG_UFS_CFG3);
> --
> 2.48.1
> 

Gentle Reminder!!