Re: [patch 05/39] genirq/msi: Remove filter from msi_free_descs_free_range()

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Thomas,

tglx@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 <tglx@linutronix.de>
> ---

[...]

> --- 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 = &dev->msi.data->__store;
>  	struct msi_desc *desc;
> @@ -155,10 +154,12 @@ void msi_free_msi_descs_range(struct dev
>  	lockdep_assert_held(&dev->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: 00000005 (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 : ffff80000a82b8d0
[    2.098745] x29: ffff80000a82b8d0 x28: 00007bfdbb508ca8 x27: ffff000102e28940
[    2.105921] x26: 0000000000000004 x25: ffff000100257660 x24: ffff800009a6b8d8
[    2.113096] x23: ffff800008a1c868 x22: ffff000102e4b700 x21: ffff000101494bb0
[    2.120270] x20: ffff80000a82b958 x19: ffff800009afe248 x18: 0000000000000010
[    2.127444] x17: fffffc0000fa4008 x16: 0000000000000008 x15: 000000000000013a
[    2.134618] x14: ffff80000a82b6a0 x13: 00000000ffffffea x12: ffff80000a82b870
[    2.141794] x11: 0000000000000002 x10: 0000000000000000 x9 : 0000000000000001
[    2.148967] x8 : 0000000000000000 x7 : 0000000000000238 x6 : ffff0001005f1230
[    2.156141] x5 : 0000000000000000 x4 : 0000200000000000 x3 : 0000000000000000
[    2.163315] x2 : b586accf01c45400 x1 : ffff0001000e0000 x0 : 000000000000002d
[    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]  deferred_probe_work_func+0x88/0xc0
[    2.251024]  process_one_work+0x1fc/0x348
[    2.255056]  worker_thread+0x228/0x420
[    2.258825]  kthread+0x10c/0x118
[    2.262075]  ret_from_fork+0x10/0x20
[    2.265672] ---[ end trace 0000000000000000 ]---

Thanks,
Miquèl