* [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()
@ 2018-09-07 15:01 Miquel Raynal
2018-09-20 18:39 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Miquel Raynal @ 2018-09-07 15:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J . Wysocki, Marc Zyngier
Cc: Thomas Petazzoni, Nadav Haklai, Gregory Clement,
Maxime Chevallier, Antoine Tenart, Stefan Chulski, Miquel Raynal,
stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()
2018-09-07 15:01 [PATCH] platform-msi: Free descriptors in platform_msi_domain_free() Miquel Raynal
@ 2018-09-20 18:39 ` Marc Zyngier
2018-09-24 13:39 ` Miquel Raynal
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2018-09-20 18:39 UTC (permalink / raw)
To: Miquel Raynal
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Thomas Petazzoni,
Nadav Haklai, Gregory Clement, Maxime Chevallier, Antoine Tenart,
Stefan Chulski, stable
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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()
2018-09-20 18:39 ` Marc Zyngier
@ 2018-09-24 13:39 ` Miquel Raynal
2018-09-28 8:34 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Miquel Raynal @ 2018-09-24 13:39 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Thomas Petazzoni,
Nadav Haklai, Gregory Clement, Maxime Chevallier, Antoine Tenart,
Stefan Chulski, stable
Hi Marc,
Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 20 Sep 2018 19:39:21
+0100:
> 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);
First I tried exactly what you propose, however
platform_msi_free_descs() takes a "base" IRQ number which is not the
Linux global virq number but the local MSI domain index.
virq (in the above example) is checked against desc->platform.msi_index
(0, 1, 2, ... 29) while in the function I wrote, virq is checked
against desc->irq (12, 13, ..., 19, 26, 27, ..., 48).
If you prefer not to add a "platform_msi_domain_free_descs()" helper,
I see another solution which is to use desc->platform.msi_index instead
of virq, and put platform_msi_free_descs() inside the
for_each_msi_entry() loop (making it _safe otherwise it would crash
for each destroyed descriptor in the list).
See the below code.
I personally prefer the function in my first proposal which avoids
calling platform_msi_free_descs() once for each descriptor to free.
diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index b9d9d1729215..fb9aa6fcdad9 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -380,17 +380,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);
+ platform_msi_free_descs(data->dev, desc->platform.msi_index, 1);
}
}
/**
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)
Thanks,
Miquèl
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()
2018-09-24 13:39 ` Miquel Raynal
@ 2018-09-28 8:34 ` Marc Zyngier
2018-09-28 15:10 ` Miquel Raynal
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2018-09-28 8:34 UTC (permalink / raw)
To: Miquel Raynal
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Thomas Petazzoni,
Nadav Haklai, Gregory Clement, Maxime Chevallier, Antoine Tenart,
Stefan Chulski, stable
Hi Miquel,
On 24/09/18 14:39, Miquel Raynal wrote:
> Hi Marc,
>
> Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 20 Sep 2018 19:39:21
> +0100:
>
>> 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);
>
> First I tried exactly what you propose, however
> platform_msi_free_descs() takes a "base" IRQ number which is not the
> Linux global virq number but the local MSI domain index.
Indeed, you're absolutely right. Apologies.
>
> virq (in the above example) is checked against desc->platform.msi_index
> (0, 1, 2, ... 29) while in the function I wrote, virq is checked
> against desc->irq (12, 13, ..., 19, 26, 27, ..., 48).
>
>
> If you prefer not to add a "platform_msi_domain_free_descs()" helper,
> I see another solution which is to use desc->platform.msi_index instead
> of virq, and put platform_msi_free_descs() inside the
> for_each_msi_entry() loop (making it _safe otherwise it would crash
> for each destroyed descriptor in the list).
>
> See the below code.
>
> I personally prefer the function in my first proposal which avoids
> calling platform_msi_free_descs() once for each descriptor to free.
>
>
>
> diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
> index b9d9d1729215..fb9aa6fcdad9 100644
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -380,17 +380,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);
> + platform_msi_free_descs(data->dev, desc->platform.msi_index, 1);
At that stage, you're better off just calling
list_del(&desc->list);
free_msi_entry(desc);
I like this approach better as we only traverse the list once.
> }
> }
>
> /**
> 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)
If you repin this, I'll queue it right away.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()
2018-09-28 8:34 ` Marc Zyngier
@ 2018-09-28 15:10 ` Miquel Raynal
[not found] ` <86k1moubgr.wl-marc.zyngier@arm.com>
0 siblings, 1 reply; 6+ messages in thread
From: Miquel Raynal @ 2018-09-28 15:10 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Thomas Petazzoni,
Nadav Haklai, Gregory Clement, Maxime Chevallier, Antoine Tenart,
Stefan Chulski, stable
Hi Marc,
[...]
> At that stage, you're better off just calling
>
> list_del(&desc->list);
> free_msi_entry(desc);
>
> I like this approach better as we only traverse the list once.
Right.
>
> > }
> > }
> > > /**
> > 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)
>
> If you repin this, I'll queue it right away.
Let me test the new version to be sure I'm not breaking anything and
I'll send a v2.
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform-msi: Free descriptors in platform_msi_domain_free()
[not found] ` <86k1moubgr.wl-marc.zyngier@arm.com>
@ 2018-10-11 9:17 ` Miquel Raynal
0 siblings, 0 replies; 6+ messages in thread
From: Miquel Raynal @ 2018-10-11 9:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Thomas Petazzoni,
Nadav Haklai, Gregory Clement, Maxime Chevallier, Antoine Tenart,
Stefan Chulski, stable
Hi Marc,
Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 11 Oct 2018 09:36:04
+0100:
> Miquel,
>
> On Fri, 28 Sep 2018 16:10:29 +0100,
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Marc,
> >
> > [...]
> >
> > > At that stage, you're better off just calling
> > >
> > > list_del(&desc->list);
> > > free_msi_entry(desc);
> > >
> > > I like this approach better as we only traverse the list once.
> >
> > Right.
> >
> > >
> > > > }
> > > > }
> > > > > /**
> > > > 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)
> > >
> > > If you repin this, I'll queue it right away.
> >
> > Let me test the new version to be sure I'm not breaking anything and
> > I'll send a v2.
>
> What is the status of this? Are you still planning to send a v2? I'd
> really like this fix to reach 4.19 before we put the last nail on it.
Sorry about that, I was sure I already sent the v2, now it's
done.
The changes in this v2 are that instead of creating a
platform_msi_domain_free_descs() helper that iterates over the list of
descriptors, the descriptor itself is removed from the list and destroyed
directly in platform_msi_domain_free(). The for_each_msi_entry() loop is
also transformed to use the "_safe" alternative.
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-11 16:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-07 15:01 [PATCH] platform-msi: Free descriptors in platform_msi_domain_free() Miquel Raynal
2018-09-20 18:39 ` Marc Zyngier
2018-09-24 13:39 ` Miquel Raynal
2018-09-28 8:34 ` Marc Zyngier
2018-09-28 15:10 ` Miquel Raynal
[not found] ` <86k1moubgr.wl-marc.zyngier@arm.com>
2018-10-11 9:17 ` Miquel Raynal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).