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

[James Bottomley]

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@xxxxxxxxxxxxx>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Cc: "James E.J. Bottomley" <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> Cc: "Martin K. Petersen" <martin.petersen@xxxxxxxxxx>
> Cc: linux-scsi@xxxxxxxxxxxxxxx
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx>