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>
