Re: [patch 05/39] genirq/msi: Remove filter from msi_free_descs_free_range()
Hi Thomas,
t...@linutronix.de wrote on Wed, 01 Mar 2023 22:07:48 +0100:
> Miquel!
>
> On Wed, Mar 01 2023 at 11:55, Miquel Raynal wrote:
> > t...@linutronix.de 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
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, );
> 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, ))
> + return;
> +
> + xa = >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
Re: [patch 05/39] genirq/msi: Remove filter from msi_free_descs_free_range()
Miquel!
On Wed, Mar 01 2023 at 11:55, Miquel Raynal wrote:
> t...@linutronix.de 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.
> desc->irq entry should be zeroed (if I understand that correctly), any
> help will be appreciated.
Untested workaround below. I hate it with a passion, but *shrug*.
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(-)
--- 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, );
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, ))
+ return;
+
+ xa = >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
Re: [patch 05/39] genirq/msi: Remove filter from msi_free_descs_free_range()
Hi Thomas,
t...@linutronix.de 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.
>
> Signed-off-by: Thomas Gleixner
> ---
[...]
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -120,7 +120,7 @@ static int msi_add_simple_msi_descs(stru
> fail_mem:
> ret = -ENOMEM;
> fail:
> - msi_free_msi_descs_range(dev, MSI_DESC_ALL, index, last);
> + msi_free_msi_descs_range(dev, index, last);
> return ret;
> }
>
> @@ -141,12 +141,11 @@ static bool msi_desc_match(struct msi_de
> /**
> * msi_free_msi_descs_range - Free MSI descriptors of a device
> * @dev: Device to free the descriptors
> - * @filter: Descriptor state filter
> * @first_index: Index to start freeing from
> * @last_index: Last index to be freed
> */
> -void msi_free_msi_descs_range(struct device *dev, enum msi_desc_filter
> filter,
> - unsigned int first_index, unsigned int last_index)
> +void msi_free_msi_descs_range(struct device *dev, unsigned int first_index,
> + unsigned int last_index)
> {
> struct xarray *xa = >msi.data->__store;
> struct msi_desc *desc;
> @@ -155,10 +154,12 @@ void msi_free_msi_descs_range(struct dev
> lockdep_assert_held(>msi.data->mutex);
>
> xa_for_each_range(xa, idx, desc, first_index, last_index) {
> - if (msi_desc_match(desc, filter)) {
> - xa_erase(xa, idx);
> - msi_free_desc(desc);
> - }
> + xa_erase(xa, idx);
> +
> + /* 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.
In my case I don't think the mvpp2_main.c driver nor the
irq_mvebu_icu.c driver behind do anything strange (as far as I
understand them). I believe any error during a ->probe() leading
to an irq_dispose_mapping() call with MSI behind will trigger that
warning.
I am wondering how useful thisd WARN_ON() is, or otherwise where the
desc->irq entry should be zeroed (if I understand that correctly), any
help will be appreciated.
Here is the splat:
[2.045857] [ cut here ]
[2.050497] WARNING: CPU: 2 PID: 9 at kernel/irq/msi.c:197
msi_domain_free_descs+0x120/0x128
[2.058993] Modules linked in:
[2.062068] CPU: 2 PID: 9 Comm: kworker/u8:0 Not tainted 6.2.0-rc1+ #168
[2.068804] Hardware name: Delta TN48M (DT)
[2.073008] Workqueue: events_unbound deferred_probe_work_func
[2.078878] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[2.085875] pc : msi_domain_free_descs+0x120/0x128
[2.090693] lr : msi_domain_free_descs+0xe0/0x128
[2.095423] sp : 8a82b8d0
[2.098745] x29: 8a82b8d0 x28: 7bfdbb508ca8 x27: 000102e28940
[2.105921] x26: 0004 x25: 000100257660 x24: 89a6b8d8
[2.113096] x23: 88a1c868 x22: 000102e4b700 x21: 000101494bb0
[2.120270] x20: 8a82b958 x19: 89afe248 x18: 0010
[2.127444] x17: fcfa4008 x16: 0008 x15: 013a
[2.134618] x14: 8a82b6a0 x13: ffea x12: 8a82b870
[2.141794] x11: 0002 x10: x9 : 0001
[2.148967] x8 : x7 : 0238 x6 : 0001005f1230
[2.156141] x5 : x4 : 2000 x3 :
[2.163315] x2 : b586accf01c45400 x1 : 0001000e x0 : 002d
[2.170489] Call trace:
[2.172948] msi_domain_free_descs+0x120/0x128
[2.177417] msi_domain_free_msi_descs_range+0x64/0x9c
[2.182586] platform_msi_device_domain_free+0x88/0xb8
[2.187752] mvebu_icu_irq_domain_free+0x60/0x80
[2.192396] irq_domain_free_irqs_hierarchy.part.21+0x94/0xa8
[2.198173] irq_domain_free_irqs+0xec/0x150
[2.202466] irq_dispose_mapping+0x104/0x120
[2.206758] mvpp2_probe+0x2028/0x21f8
[2.210530] platform_probe+0x68/0xd8
[2.214210] really_probe+0xbc/0x2a8
[2.217807] __driver_probe_device+0x78/0xe0
[2.222102] driver_probe_device+0x3c/0x108
[2.226308] __device_attach_driver+0xb8/0xf8
[2.230690] bus_for_each_drv+0x7c/0xd0
[2.234547] __device_attach+0xec/0x188
[2.238404] device_initial_probe+0x14/0x20
[2.242611] bus_probe_device+0x9c/0xa8
[2.246468]
Re: [patch 05/39] genirq/msi: Remove filter from msi_free_descs_free_range()
On Fri, Nov 11, 2022 at 02:54:22PM +0100, Thomas Gleixner wrote: > 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. > > Signed-off-by: Thomas Gleixner > --- > drivers/base/platform-msi.c |2 +- > include/linux/msi.h |5 ++--- > kernel/irq/msi.c| 19 ++- > 3 files changed, 13 insertions(+), 13 deletions(-) Reviewed-by: Jason Gunthorpe Jason
