Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition
On 28/02/18 09:25, Shah, Amit wrote:
>
> On Mi, 2018-02-28 at 08:16 +, Roger Pau Monné wrote:
>> On Tue, Feb 27, 2018 at 05:32:53PM +, Shah, Amit wrote:
>>>
>>>
>>> On Di, 2018-02-27 at 17:07 +, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
>
>
> In case of errors in irq setup for MSI, free up the allocated
> irqs.
>
> Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message
> groups")
> Reported-by: Hooman Mirhadi
> CC:
> CC: Roger Pau Monné
> CC: Boris Ostrovsky
> CC: Eduardo Valentin
> CC: Juergen Gross
> CC: Thomas Gleixner
> CC: "K. Y. Srinivasan"
> CC: Liu Shuo
> CC: Anoob Soman
> Signed-off-by: Amit Shah
> ---
> drivers/xen/events/events_base.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/events/events_base.c
> b/drivers/xen/events/events_base.c
> index c86d10e..a299586 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct
> pci_dev
> *dev, struct msi_desc *msidesc,
>
> ret = irq_set_msi_desc(irq, msidesc);
> if (ret < 0)
> - goto error_irq;
> + goto error_desc;
> out:
> mutex_unlock(_mapping_update_lock);
> return irq;
> error_irq:
> + while (--nvec >= i)
> + xen_free_irq(irq + nvec);
> +error_desc:
> while (i > 0) {
> i--;
> __unbind_from_irq(irq + i);
It seems pointless to introduce another label and another loop to
fix
something that can be fixed with a single label and a single
loop,
this just makes the code more complex for no reason.
>>> I disagree, just because there are two different cleanups to be
>>> made
>>> for two different issues; it's not as if the if.. and else
>>> conditions
>>> are going to be interleaved.
>> Oh, I don't mind so much whether it ends up being two patches or a
>> single one, but IMHO the code should end up looking similar to what I
>> proposed, I would like to avoid having two loops and two labels.
>>
>> Could you rework the series so that the end result uses a single loop
>> (and label)?
>
> That was the part I didn't like much, so it would be better if the
> patch came from you :)
I'd prefer Roger's solution, too.
Roger, in case you don't want to write the patch, I can do it.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition
On Mi, 2018-02-28 at 08:16 +, Roger Pau Monné wrote:
> On Tue, Feb 27, 2018 at 05:32:53PM +, Shah, Amit wrote:
> >
> >
> > On Di, 2018-02-27 at 17:07 +, Roger Pau Monné wrote:
> > >
> > > On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
> > > >
> > > >
> > > > In case of errors in irq setup for MSI, free up the allocated
> > > > irqs.
> > > >
> > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message
> > > > groups")
> > > > Reported-by: Hooman Mirhadi
> > > > CC:
> > > > CC: Roger Pau Monné
> > > > CC: Boris Ostrovsky
> > > > CC: Eduardo Valentin
> > > > CC: Juergen Gross
> > > > CC: Thomas Gleixner
> > > > CC: "K. Y. Srinivasan"
> > > > CC: Liu Shuo
> > > > CC: Anoob Soman
> > > > Signed-off-by: Amit Shah
> > > > ---
> > > > drivers/xen/events/events_base.c | 5 -
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/xen/events/events_base.c
> > > > b/drivers/xen/events/events_base.c
> > > > index c86d10e..a299586 100644
> > > > --- a/drivers/xen/events/events_base.c
> > > > +++ b/drivers/xen/events/events_base.c
> > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct
> > > > pci_dev
> > > > *dev, struct msi_desc *msidesc,
> > > >
> > > > ret = irq_set_msi_desc(irq, msidesc);
> > > > if (ret < 0)
> > > > - goto error_irq;
> > > > + goto error_desc;
> > > > out:
> > > > mutex_unlock(_mapping_update_lock);
> > > > return irq;
> > > > error_irq:
> > > > + while (--nvec >= i)
> > > > + xen_free_irq(irq + nvec);
> > > > +error_desc:
> > > > while (i > 0) {
> > > > i--;
> > > > __unbind_from_irq(irq + i);
> > > It seems pointless to introduce another label and another loop to
> > > fix
> > > something that can be fixed with a single label and a single
> > > loop,
> > > this just makes the code more complex for no reason.
> > I disagree, just because there are two different cleanups to be
> > made
> > for two different issues; it's not as if the if.. and else
> > conditions
> > are going to be interleaved.
> Oh, I don't mind so much whether it ends up being two patches or a
> single one, but IMHO the code should end up looking similar to what I
> proposed, I would like to avoid having two loops and two labels.
>
> Could you rework the series so that the end result uses a single loop
> (and label)?
That was the part I didn't like much, so it would be better if the
patch came from you :)
Amit
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition
On Tue, Feb 27, 2018 at 05:32:53PM +, Shah, Amit wrote:
>
> On Di, 2018-02-27 at 17:07 +, Roger Pau Monné wrote:
> > On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
> > >
> > > In case of errors in irq setup for MSI, free up the allocated irqs.
> > >
> > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> > > Reported-by: Hooman Mirhadi
> > > CC:
> > > CC: Roger Pau Monné
> > > CC: Boris Ostrovsky
> > > CC: Eduardo Valentin
> > > CC: Juergen Gross
> > > CC: Thomas Gleixner
> > > CC: "K. Y. Srinivasan"
> > > CC: Liu Shuo
> > > CC: Anoob Soman
> > > Signed-off-by: Amit Shah
> > > ---
> > > drivers/xen/events/events_base.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/xen/events/events_base.c
> > > b/drivers/xen/events/events_base.c
> > > index c86d10e..a299586 100644
> > > --- a/drivers/xen/events/events_base.c
> > > +++ b/drivers/xen/events/events_base.c
> > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev
> > > *dev, struct msi_desc *msidesc,
> > >
> > > ret = irq_set_msi_desc(irq, msidesc);
> > > if (ret < 0)
> > > - goto error_irq;
> > > + goto error_desc;
> > > out:
> > > mutex_unlock(_mapping_update_lock);
> > > return irq;
> > > error_irq:
> > > + while (--nvec >= i)
> > > + xen_free_irq(irq + nvec);
> > > +error_desc:
> > > while (i > 0) {
> > > i--;
> > > __unbind_from_irq(irq + i);
> > It seems pointless to introduce another label and another loop to fix
> > something that can be fixed with a single label and a single loop,
> > this just makes the code more complex for no reason.
>
> I disagree, just because there are two different cleanups to be made
> for two different issues; it's not as if the if.. and else conditions
> are going to be interleaved.
Oh, I don't mind so much whether it ends up being two patches or a
single one, but IMHO the code should end up looking similar to what I
proposed, I would like to avoid having two loops and two labels.
Could you rework the series so that the end result uses a single loop
(and label)?
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition
On Di, 2018-02-27 at 17:07 +, Roger Pau Monné wrote:
> On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
> >
> > In case of errors in irq setup for MSI, free up the allocated irqs.
> >
> > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> > Reported-by: Hooman Mirhadi
> > CC:
> > CC: Roger Pau Monné
> > CC: Boris Ostrovsky
> > CC: Eduardo Valentin
> > CC: Juergen Gross
> > CC: Thomas Gleixner
> > CC: "K. Y. Srinivasan"
> > CC: Liu Shuo
> > CC: Anoob Soman
> > Signed-off-by: Amit Shah
> > ---
> > drivers/xen/events/events_base.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/events/events_base.c
> > b/drivers/xen/events/events_base.c
> > index c86d10e..a299586 100644
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev
> > *dev, struct msi_desc *msidesc,
> >
> > ret = irq_set_msi_desc(irq, msidesc);
> > if (ret < 0)
> > - goto error_irq;
> > + goto error_desc;
> > out:
> > mutex_unlock(_mapping_update_lock);
> > return irq;
> > error_irq:
> > + while (--nvec >= i)
> > + xen_free_irq(irq + nvec);
> > +error_desc:
> > while (i > 0) {
> > i--;
> > __unbind_from_irq(irq + i);
> It seems pointless to introduce another label and another loop to fix
> something that can be fixed with a single label and a single loop,
> this just makes the code more complex for no reason.
I disagree, just because there are two different cleanups to be made
for two different issues; it's not as if the if.. and else conditions
are going to be interleaved.
Anyway it's a matter of taste.
Since you've already proposed the patch, would you mind baking a proper
one and posting it?
Thanks!
> IMHO the way to solve this issue is:
>
> while (nvec--) {
> if (nvec >= i)
> xen_free_irq(irq + nvec);
> else
> __unbind_from_irq(irq + nvec);
> }
Amit
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition
On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
> In case of errors in irq setup for MSI, free up the allocated irqs.
>
> Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> Reported-by: Hooman Mirhadi
> CC:
> CC: Roger Pau Monné
> CC: Boris Ostrovsky
> CC: Eduardo Valentin
> CC: Juergen Gross
> CC: Thomas Gleixner
> CC: "K. Y. Srinivasan"
> CC: Liu Shuo
> CC: Anoob Soman
> Signed-off-by: Amit Shah
> ---
> drivers/xen/events/events_base.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/events/events_base.c
> b/drivers/xen/events/events_base.c
> index c86d10e..a299586 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev,
> struct msi_desc *msidesc,
>
> ret = irq_set_msi_desc(irq, msidesc);
> if (ret < 0)
> - goto error_irq;
> + goto error_desc;
> out:
> mutex_unlock(_mapping_update_lock);
> return irq;
> error_irq:
> + while (--nvec >= i)
> + xen_free_irq(irq + nvec);
> +error_desc:
> while (i > 0) {
> i--;
> __unbind_from_irq(irq + i);
It seems pointless to introduce another label and another loop to fix
something that can be fixed with a single label and a single loop,
this just makes the code more complex for no reason.
IMHO the way to solve this issue is:
while (nvec--) {
if (nvec >= i)
xen_free_irq(irq + nvec);
else
__unbind_from_irq(irq + nvec);
}
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition
In case of errors in irq setup for MSI, free up the allocated irqs.
Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi
CC:
CC: Roger Pau Monné
CC: Boris Ostrovsky
CC: Eduardo Valentin
CC: Juergen Gross
CC: Thomas Gleixner
CC: "K. Y. Srinivasan"
CC: Liu Shuo
CC: Anoob Soman
Signed-off-by: Amit Shah
---
drivers/xen/events/events_base.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index c86d10e..a299586 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct
msi_desc *msidesc,
ret = irq_set_msi_desc(irq, msidesc);
if (ret < 0)
- goto error_irq;
+ goto error_desc;
out:
mutex_unlock(_mapping_update_lock);
return irq;
error_irq:
+ while (--nvec >= i)
+ xen_free_irq(irq + nvec);
+error_desc:
while (i > 0) {
i--;
__unbind_from_irq(irq + i);
--
2.7.3.AMZN
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
