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

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

[Miquel Raynal]

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 | 6 ++++--
 include/linux/msi.h         | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 60d6cc618f1c..6d54905c6263 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -366,14 +366,16 @@ void platform_msi_domain_free(struct irq_domain *domain, unsigned int virq,
 			      unsigned int nvec)
 {
 	struct platform_msi_priv_data *data = domain->host_data;
-	struct msi_desc *desc;
-	for_each_msi_entry(desc, data->dev) {
+	struct msi_desc *desc, *tmp;
+	for_each_msi_entry_safe(desc, tmp, data->dev) {
 		if (WARN_ON(!desc->irq || desc->nvec_used != 1))
 			return;
 		if (!(desc->irq >= virq && desc->irq < (virq + nvec)))
 			continue;
 
 		irq_domain_free_irqs_common(domain, desc->irq, 1);
+		list_del(&desc->list);
+		free_msi_entry(desc);
 	}
 }
 
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 5839d8062dfc..be8ec813dbfb 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -116,6 +116,8 @@ struct msi_desc {
 	list_first_entry(dev_to_msi_list((dev)), struct msi_desc, list)
 #define for_each_msi_entry(desc, dev)	\
 	list_for_each_entry((desc), dev_to_msi_list((dev)), list)
+#define for_each_msi_entry_safe(desc, tmp, dev)	\
+	list_for_each_entry_safe((desc), (tmp), dev_to_msi_list((dev)), list)
 
 #ifdef CONFIG_PCI_MSI
 #define first_pci_msi_entry(pdev)	first_msi_entry(&(pdev)->dev)
-- 
2.17.1

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

[Marc Zyngier]

Hi Miquel,

On 11/10/18 10:12, Miquel Raynal 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>

Thanks for the respin. If nobody disagrees, I'll route this through 
Thomas to stash into tip, and hopefully have that in 4.19.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Hanjun Guo]

Hi Marc, Miquel,

On 2018/10/12 18:24, Marc Zyngier wrote:
> Hi Miquel,
> 
> On 11/10/18 10:12, Miquel Raynal 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>
> 
> Thanks for the respin. If nobody disagrees, I'll route this through Thomas to stash into tip, and hopefully have that in 4.19.

I didn't see this patch hit mainline, did I miss something?

Thanks
Hanjun

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

[Miquel Raynal]

Hi Marc, Hanjun,

Hanjun Guo <guohanjun@huawei.com> wrote on Mon, 12 Nov 2018 11:30:42
+0800:

> Hi Marc, Miquel,
> 
> On 2018/10/12 18:24, Marc Zyngier wrote:
> > Hi Miquel,
> > 
> > On 11/10/18 10:12, Miquel Raynal 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>  
> > 
> > Thanks for the respin. If nobody disagrees, I'll route this through Thomas to stash into tip, and hopefully have that in 4.19.  
> 
> I didn't see this patch hit mainline, did I miss something?

Indeed, I can't see it neither. Marc, is it possible to have it queued
for your next PR?

Miquèl

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

[Miquel Raynal]

Hi Marc,

Hanjun Guo <guohanjun@huawei.com> wrote on Mon, 12 Nov 2018 11:30:42
+0800:

> Free descriptors in  platform_msi_domain_free

Sorry for pinging you again but I still do not find this patch in any
official tree, will it be part of 4.20?

Thanks,
Miquèl