Hi Miquel,
On Fri, 07 Sep 2018 16:01:29 +0100,
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Since the addition of platform MSI support, there were two helpers
> supposed to allocate/free IRQs for a device:
>
> platform_msi_domain_alloc_irqs()
> platform_msi_domain_free_irqs()
>
> In these helpers, IRQ descriptors are allocated in the "alloc" routine
> while they are freed in the "free" one.
>
> Later, two other helpers have been added to handle IRQ domains on top
> of MSI domains:
>
> platform_msi_domain_alloc()
> platform_msi_domain_free()
>
> Seen from the outside, the logic is pretty close with the former
> helpers and people used it with the same logic as before: a
> platform_msi_domain_alloc() call should be balanced with a
> platform_msi_domain_free() call. While this is probably what was
> intended to do, the platform_msi_domain_free() does not remove/free
> the IRQ descriptor(s) created/inserted in
> platform_msi_domain_alloc().
>
> One effect of such situation is that removing a module that requested
> an IRQ will let one orphaned IRQ descriptor (with an allocated MSI
> entry) in the device descriptors list. Next time the module will be
> inserted back, one will observe that the allocation will happen twice
> in the MSI domain, one time for the remaining descriptor, one time for
> the new one. It also has the side effect to quickly overshoot the
> maximum number of allocated MSI and then prevent any module requesting
> an interrupt in the same domain to be inserted anymore.
>
> This situation has been met with loops of insertion/removal of the
> mvpp2.ko module (requesting 15 MSIs each time).
>
> Fixes: 552c494a7666 ("platform-msi: Allow creation of a MSI-based stacked irq domain")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
> drivers/base/platform-msi.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
> index 60d6cc618f1c..b9d9d1729215 100644
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -354,6 +354,20 @@ platform_msi_create_device_domain(struct device *dev,
> return NULL;
> }
>
> +static void platform_msi_domain_free_descs(struct irq_domain *domain, int virq,
> + int nvec)
> +{
> + struct platform_msi_priv_data *data = domain->host_data;
> + struct msi_desc *desc, *tmp;
> +
> + list_for_each_entry_safe(desc, tmp, dev_to_msi_list(data->dev), list) {
> + if (desc->irq >= virq && desc->irq < (virq + nvec)) {
> + list_del(&desc->list);
> + free_msi_entry(desc);
> + }
> + }
> +}
> +
> /**
> * platform_msi_domain_free - Free interrupts associated with a platform-msi
> * domain
> @@ -375,6 +389,8 @@ void platform_msi_domain_free(struct irq_domain *domain, unsigned int virq,
>
> irq_domain_free_irqs_common(domain, desc->irq, 1);
> }
> +
> + platform_msi_domain_free_descs(domain, virq, nvec);
> }
>
> /**
> --
> 2.17.1
>
Good catch, but I wonder why you don't use the existing
helper instead. Something like this (untested):
diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 60d6cc618f1c..87808ac08bfb 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -375,6 +375,8 @@ void platform_msi_domain_free(struct irq_domain *domain, unsigned int virq,
irq_domain_free_irqs_common(domain, desc->irq, 1);
}
+
+ platform_msi_free_descs(data->dev, virq, nvec);
}
/**
Thanks,
M.
--
Jazz is not dead, it just smell funny.
