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 mirhadih@amazon.com CC: stable@vger.kernel.org CC: Roger Pau Monné roger.pau@citrix.com CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Eduardo Valentin eduval@amazon.com CC: Juergen Gross jgross@suse.com CC: Thomas Gleixner tglx@linutronix.de CC: "K. Y. Srinivasan" kys@microsoft.com CC: Liu Shuo shuo.a.liu@intel.com CC: Anoob Soman anoob.soman@citrix.com Signed-off-by: Amit Shah aams@amazon.com --- 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(&irq_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);
On Tue, Feb 27, 2018 at 03:55:58PM +0000, 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 mirhadih@amazon.com CC: stable@vger.kernel.org CC: Roger Pau Monné roger.pau@citrix.com CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Eduardo Valentin eduval@amazon.com CC: Juergen Gross jgross@suse.com CC: Thomas Gleixner tglx@linutronix.de CC: "K. Y. Srinivasan" kys@microsoft.com CC: Liu Shuo shuo.a.liu@intel.com CC: Anoob Soman anoob.soman@citrix.com Signed-off-by: Amit Shah aams@amazon.com
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(&irq_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.
On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 03:55:58PM +0000, 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 mirhadih@amazon.com CC: stable@vger.kernel.org CC: Roger Pau Monné roger.pau@citrix.com CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Eduardo Valentin eduval@amazon.com CC: Juergen Gross jgross@suse.com CC: Thomas Gleixner tglx@linutronix.de CC: "K. Y. Srinivasan" kys@microsoft.com CC: Liu Shuo shuo.a.liu@intel.com CC: Anoob Soman anoob.soman@citrix.com Signed-off-by: Amit Shah aams@amazon.com
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(&irq_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
On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote:
On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 03:55:58PM +0000, 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 mirhadih@amazon.com CC: stable@vger.kernel.org CC: Roger Pau Monné roger.pau@citrix.com CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Eduardo Valentin eduval@amazon.com CC: Juergen Gross jgross@suse.com CC: Thomas Gleixner tglx@linutronix.de CC: "K. Y. Srinivasan" kys@microsoft.com CC: Liu Shuo shuo.a.liu@intel.com CC: Anoob Soman anoob.soman@citrix.com Signed-off-by: Amit Shah aams@amazon.com
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(&irq_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.
On Mi, 2018-02-28 at 08:16 +0000, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote:
On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 03:55:58PM +0000, 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 mirhadih@amazon.com CC: stable@vger.kernel.org CC: Roger Pau Monné roger.pau@citrix.com CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Eduardo Valentin eduval@amazon.com CC: Juergen Gross jgross@suse.com CC: Thomas Gleixner tglx@linutronix.de CC: "K. Y. Srinivasan" kys@microsoft.com CC: Liu Shuo shuo.a.liu@intel.com CC: Anoob Soman anoob.soman@citrix.com Signed-off-by: Amit Shah aams@amazon.com
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(&irq_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
On 28/02/18 09:25, Shah, Amit wrote:
On Mi, 2018-02-28 at 08:16 +0000, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote:
On Di, 2018-02-27 at 17:07 +0000, Roger Pau Monné wrote:
On Tue, Feb 27, 2018 at 03:55:58PM +0000, 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 mirhadih@amazon.com CC: stable@vger.kernel.org CC: Roger Pau Monné roger.pau@citrix.com CC: Boris Ostrovsky boris.ostrovsky@oracle.com CC: Eduardo Valentin eduval@amazon.com CC: Juergen Gross jgross@suse.com CC: Thomas Gleixner tglx@linutronix.de CC: "K. Y. Srinivasan" kys@microsoft.com CC: Liu Shuo shuo.a.liu@intel.com CC: Anoob Soman anoob.soman@citrix.com Signed-off-by: Amit Shah aams@amazon.com
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(&irq_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
linux-stable-mirror@lists.linaro.org
-
Amit Shah -
Juergen Gross -
Roger Pau Monné -
Shah, Amit