Hi Thomas,
tglx@xxxxxxxxxxxxx wrote on Wed, 01 Mar 2023 22:07:48 +0100:
> Miquel!
>
> On Wed, Mar 01 2023 at 11:55, Miquel Raynal wrote:
> > tglx@xxxxxxxxxxxxx wrote on Fri, 11 Nov 2022 14:54:22 +0100 (CET):
> >
> >> When a range of descriptors is freed then all of them are not associated to
> >> a linux interrupt. Remove the filter and add a warning to the free function.
> >> + /* Leak the descriptor when it is still referenced */
> >> + if (WARN_ON_ONCE(msi_desc_match(desc, MSI_DESC_ASSOCIATED)))
> >> + continue;
> >> + msi_free_desc(desc);
> >> }
> >> }
> >
> > It looks like since this commit I am getting warnings upon EPROBE_DEFER
> > errors in the mvpp2 Marvell Ethernet driver. I looked a bit at the
> > internals to understand why this warning was shown, and it seems that
> > nothing "de-references" the descriptors, which would mean here:
> > resetting desc->irq to 0.
>
> Correct. This platform-msi ^(*&!@&^ hack really needs to die ASAP.
:-)
> Marc, where are we on that? Is this still in limbo?
>
> > I am wondering how useful thisd WARN_ON() is, or otherwise where the
>
> It is useful as it caught bugs already.
Sure.
> > desc->irq entry should be zeroed (if I understand that correctly), any
> > help will be appreciated.
>
> Untested workaround below.
Excellent!
> I hate it with a passion, but *shrug*.
:'D
> Thanks,
>
> tglx
> ---
> drivers/base/platform-msi.c | 1 +
> include/linux/msi.h | 2 ++
> kernel/irq/msi.c | 23 ++++++++++++++++++++++-
> 3 files changed, 25 insertions(+), 1 deletion(-)
Kudos for the diff, which indeed works perfectly in my case. I guess
you'll make a proper patch soon, if that's the case you can add my:
Tested-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
Let me know otherwise.
Thanks a lot for the very quick fix!
Miquèl
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -324,6 +324,7 @@ void platform_msi_device_domain_free(str
> struct platform_msi_priv_data *data = domain->host_data;
>
> msi_lock_descs(data->dev);
> + msi_domain_depopulate_descs(data->dev, virq, nr_irqs);
> irq_domain_free_irqs_common(domain, virq, nr_irqs);
> msi_free_msi_descs_range(data->dev, virq, virq + nr_irqs - 1);
> msi_unlock_descs(data->dev);
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -631,6 +631,8 @@ int msi_domain_prepare_irqs(struct irq_d
> int nvec, msi_alloc_info_t *args);
> int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
> int virq, int nvec, msi_alloc_info_t *args);
> +void msi_domain_depopulate_descs(struct device *dev, int virq, int nvec);
> +
> struct irq_domain *
> __platform_msi_create_device_domain(struct device *dev,
> unsigned int nvec,
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -1109,14 +1109,35 @@ int msi_domain_populate_irqs(struct irq_
> return 0;
>
> fail:
> - for (--virq; virq >= virq_base; virq--)
> + for (--virq; virq >= virq_base; virq--) {
> + msi_domain_depopulate_descs(dev, virq, 1);
> irq_domain_free_irqs_common(domain, virq, 1);
> + }
> msi_domain_free_descs(dev, &ctrl);
> unlock:
> msi_unlock_descs(dev);
> return ret;
> }
>
> +void msi_domain_depopulate_descs(struct device *dev, int virq_base, int nvec)
> +{
> + struct msi_ctrl ctrl = {
> + .domid = MSI_DEFAULT_DOMAIN,
> + .first = virq_base,
> + .last = virq_base + nvec - 1,
> + };
> + struct msi_desc *desc;
> + struct xarray *xa;
> + unsigned long idx;
> +
> + if (!msi_ctrl_valid(dev, &ctrl))
> + return;
> +
> + xa = &dev->msi.data->__domains[ctrl.domid].store;
> + xa_for_each_range(xa, idx, desc, ctrl.first, ctrl.last)
> + desc->irq = 0;
> +}
> +
> /*
> * Carefully check whether the device can use reservation mode. If
> * reservation mode is enabled then the early activation will assign a
