Re: [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()

Hi Miquel,

On Fri, 07 Sep 2018 16:01:29 +0100, Miquel Raynal miquel.raynal@bootlin.com 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@vger.kernel.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com

---

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.