On Mon, 2025-03-17 at 14:29 +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.
>
> No functional change intended.
>
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> Cc: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org>
> Cc: "James E.J. Bottomley" <james.bottom...@hansenpartnership.com>
> Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
> Cc: linux-s...@vger.kernel.org
>
>
> ---
> drivers/ufs/host/ufs-qcom.c | 77 ++++++++++++++++++++++-----------
> -----------
> 1 file changed, 40 insertions(+), 37 deletions(-)
>
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1782,15 +1782,19 @@ 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;
> @@ -1799,8 +1803,7 @@ static irqreturn_t ufs_qcom_mcq_esi_hand
> static int ufs_qcom_config_esi(struct ufs_hba *hba)
> {
> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> - struct msi_desc *desc;
> - struct msi_desc *failed_desc = NULL;
> + struct ufs_qcom_irq *qi;
> int nr_irqs, ret;
>
> if (host->esi_enabled)
> @@ -1811,47 +1814,47 @@ static int ufs_qcom_config_esi(struct uf
> * 2. Poll queues do not need ESI.
> */
> nr_irqs = hba->nr_hw_queues - hba-
> >nr_queues[HCTX_TYPE_POLL];
> + qi = devm_kcalloc(hba->dev, nr_irqs, sizeof(*qi),
> GFP_KERNEL);
> + if (qi)
Typo causing logic inversion: should be !qi here (you need a more
responsive ! key).
> + return -ENOMEM;
> +
> 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;
> + goto cleanup;
> }
>
> - msi_lock_descs(hba->dev);
> - msi_for_each_desc(desc, hba->dev, MSI_DESC_ALL) {
> - ret = devm_request_irq(hba->dev, desc->irq,
> - ufs_qcom_mcq_esi_handler,
> - IRQF_SHARED, "qcom-mcq-esi",
> desc);
> + for (int idx = 0; idx < nr_irqs; idx++) {
> + qi[idx].irq = msi_get_virq(hba->dev, idx);
> + qi[idx].idx = idx;
> + qi[idx].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",
> - __func__, desc->irq, ret);
> - failed_desc = desc;
> - break;
> + __func__, qi[idx].irq, ret);
> + qi[idx].irq = 0;
> + goto cleanup;
> }
> }
> - msi_unlock_descs(hba->dev);
>
> - if (ret) {
> - /* Rewind */
> - msi_lock_descs(hba->dev);
> - msi_for_each_desc(desc, hba->dev, MSI_DESC_ALL) {
> - if (desc == failed_desc)
> - break;
> - devm_free_irq(hba->dev, desc->irq, hba);
> - }
> - msi_unlock_descs(hba->dev);
> - platform_device_msi_free_irqs_all(hba->dev);
> - } else {
> - if (host->hw_ver.major == 6 && host->hw_ver.minor ==
> 0 &&
> - host->hw_ver.step == 0)
> - ufshcd_rmwl(hba, ESI_VEC_MASK,
> - FIELD_PREP(ESI_VEC_MASK,
> MAX_ESI_VEC - 1),
> - REG_UFS_CFG3);
> - ufshcd_mcq_enable_esi(hba);
> - host->esi_enabled = true;
> + if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
> + host->hw_ver.step == 0) {
> + ufshcd_rmwl(hba, ESI_VEC_MASK,
> + FIELD_PREP(ESI_VEC_MASK, MAX_ESI_VEC -
> 1),
> + REG_UFS_CFG3);
> }
> -
> + ufshcd_mcq_enable_esi(hba);
> + host->esi_enabled = true;
> + return 0;
> +
> +cleanup:
> + for (int idx = 0; qi[idx].irq; idx++)
> + devm_free_irq(hba->dev, qi[idx].irq, hba);
> + platform_device_msi_free_irqs_all(hba->dev);
> + devm_kfree(hba->dev, qi);
> return ret;
> }
This does seem to be exactly the pattern you describe in 1/10, although
I'm not entirely convinced that something like the below is more
readable and safe.
Regards,
James
---
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 23b9f6efa047..26b0c665c3b7 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1782,25 +1782,37 @@ static void ufs_qcom_write_msi_msg(struct msi_desc
*desc, struct msi_msg *msg)
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;
}
+DEFINE_FREE(ufs_qcom_irq, struct ufs_qcom_irq *,
+ if (_T) { \
+ for (int idx = 0; _T[idx].irq; idx++) \
+ devm_free_irq(_T[idx].hba->dev, _T[idx].irq, \
+ _T[idx].hba); \
+ platform_device_msi_free_irqs_all(_T[0].hba->dev); \
+ devm_kfree(_T[0].hba->dev, _T); \
+ })
+
static int ufs_qcom_config_esi(struct ufs_hba *hba)
{
struct ufs_qcom_host *host = ufshcd_get_variant(hba);
- struct msi_desc *desc;
- struct msi_desc *failed_desc = NULL;
+ struct ufs_qcom_irq *qi __free(ufs_qcom_irq);
int nr_irqs, ret;
if (host->esi_enabled)
@@ -1811,6 +1823,11 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
* 2. Poll queues do not need ESI.
*/
nr_irqs = hba->nr_hw_queues - hba->nr_queues[HCTX_TYPE_POLL];
+ qi = devm_kcalloc(hba->dev, nr_irqs, sizeof(*qi), GFP_KERNEL);
+ if (!qi)
+ return -ENOMEM;
+ qi[0].hba = hba; /* required by __free() */
+
ret = platform_device_msi_init_and_alloc_irqs(hba->dev, nr_irqs,
ufs_qcom_write_msi_msg);
if (ret) {
@@ -1818,41 +1835,31 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
return ret;
}
- msi_lock_descs(hba->dev);
- msi_for_each_desc(desc, hba->dev, MSI_DESC_ALL) {
- ret = devm_request_irq(hba->dev, desc->irq,
- ufs_qcom_mcq_esi_handler,
- IRQF_SHARED, "qcom-mcq-esi", desc);
+ for (int idx = 0; idx < nr_irqs; idx++) {
+ qi[idx].irq = msi_get_virq(hba->dev, idx);
+ qi[idx].idx = idx;
+ qi[idx].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",
- __func__, desc->irq, ret);
- failed_desc = desc;
- break;
+ __func__, qi[idx].irq, ret);
+ qi[idx].irq = 0;
+ return ret;
}
}
- msi_unlock_descs(hba->dev);
+ retain_ptr(qi);
- if (ret) {
- /* Rewind */
- msi_lock_descs(hba->dev);
- msi_for_each_desc(desc, hba->dev, MSI_DESC_ALL) {
- if (desc == failed_desc)
- break;
- devm_free_irq(hba->dev, desc->irq, hba);
- }
- msi_unlock_descs(hba->dev);
- platform_device_msi_free_irqs_all(hba->dev);
- } else {
- if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
- host->hw_ver.step == 0)
- ufshcd_rmwl(hba, ESI_VEC_MASK,
- FIELD_PREP(ESI_VEC_MASK, MAX_ESI_VEC - 1),
- REG_UFS_CFG3);
- ufshcd_mcq_enable_esi(hba);
- host->esi_enabled = true;
+ if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
+ host->hw_ver.step == 0) {
+ ufshcd_rmwl(hba, ESI_VEC_MASK,
+ FIELD_PREP(ESI_VEC_MASK, MAX_ESI_VEC - 1),
+ REG_UFS_CFG3);
}
-
- return ret;
+ ufshcd_mcq_enable_esi(hba);
+ host->esi_enabled = true;
+ return 0;
}
/*
