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

[Marc Zyngier <marc.zyngier@arm.com>]

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.

-- 
Jazz is not dead, it just smell funny.