linux-kernel - Re: [patch V4 13/14] scsi: ufs: qcom: Remove the MSI descriptor abuse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b64fc052da27c0e77afae1db40c8d3b05277cc86.camel@HansenPartnership.com>
Date: Wed, 19 Mar 2025 16:40:22 -0400
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>
Cc: Marc Zyngier <maz@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, "Martin K.
 Petersen" <martin.petersen@...cle.com>,  linux-scsi@...r.kernel.org,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,  Nishanth Menon
 <nm@...com>, Dhruva Gole <d-gole@...com>, Tero Kristo <kristo@...nel.org>, 
 Santosh Shilimkar <ssantosh@...nel.org>, Logan Gunthorpe
 <logang@...tatee.com>, Dave Jiang <dave.jiang@...el.com>,  Jon Mason
 <jdmason@...zu.us>, Allen Hubbe <allenbh@...il.com>, ntb@...ts.linux.dev,
 Michael Kelley <mhklinux@...look.com>, Wei Liu <wei.liu@...nel.org>, Bjorn
 Helgaas <bhelgaas@...gle.com>, Haiyang Zhang <haiyangz@...rosoft.com>, 
 linux-hyperv@...r.kernel.org, linux-pci@...r.kernel.org, Wei Huang
 <wei.huang2@....com>, Jonathan Cameron <Jonathan.Cameron@...ei.com>
Subject: Re: [patch V4 13/14] scsi: ufs: qcom: Remove the MSI descriptor
 abuse

On Wed, 2025-03-19 at 11:57 +0100, Thomas Gleixner wrote:
> The driver abuses the MSI descriptors for internal purposes. Aside of
> core code and MSI providers nothing has to care about their
> existence. They have been encapsulated with a lot of effort because
> this kind of abuse caused all sorts of issues including a
> maintainability nightmare.
> 
> Rewrite the code so it uses dedicated storage to hand the required
> information to the interrupt handler and use a custom cleanup
> function to simplify the error path.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> Cc: "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>
> Cc: "Martin K. Petersen" <martin.petersen@...cle.com>
> Cc: linux-scsi@...r.kernel.org
> ---
> V4: Fix inverse NULL pointer check and use __free() for error paths -
> James
> ---
>  drivers/ufs/host/ufs-qcom.c |   85 ++++++++++++++++++++++++---------
> -----------
>  1 file changed, 48 insertions(+), 37 deletions(-)
> 
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1782,25 +1782,38 @@ static void ufs_qcom_write_msi_msg(struc
>  	ufshcd_mcq_config_esi(hba, msg);
>  }
>  
> +struct ufs_qcom_irq {
> +	unsigned int		irq;
> +	unsigned int		idx;
> +	struct ufs_hba		*hba;
> +};
> +
>  static irqreturn_t ufs_qcom_mcq_esi_handler(int irq, void *data)
>  {
> -	struct msi_desc *desc = data;
> -	struct device *dev = msi_desc_to_dev(desc);
> -	struct ufs_hba *hba = dev_get_drvdata(dev);
> -	u32 id = desc->msi_index;
> -	struct ufs_hw_queue *hwq = &hba->uhq[id];
> +	struct ufs_qcom_irq *qi = data;
> +	struct ufs_hba *hba = qi->hba;
> +	struct ufs_hw_queue *hwq = &hba->uhq[qi->idx];
>  
> -	ufshcd_mcq_write_cqis(hba, 0x1, id);
> +	ufshcd_mcq_write_cqis(hba, 0x1, qi->idx);
>  	ufshcd_mcq_poll_cqe_lock(hba, hwq);
>  
>  	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))

So if every __free body basically has a condition check and an external
call, it would be a lot nicer if the macro were coded as something like

#define DEFINE_FREE(_name, _type, _cond, _free) \
	static inline void __free_##_name(void *p) { _type _T =
*(_type *)p; if (_cond(_T)) _free(_T); }

to avoid having to splash the strange _T across the kernel.

Other than that (which isn't your problem, so you can ignore it), the
code looks fine but I can't test it.

Reviewed-by: James Bottomley <James.Bottomley@...senPartnership.com>