All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] xen: fix bugs in error conditions
@ 2018-02-27 15:55 Amit Shah
  2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw)
  To: xen-devel
  Cc: linux-kernel, Amit Shah, Roger Pau Monné,
	Boris Ostrovsky, Eduardo Valentin, Juergen Gross,
	Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman

Hello,

These bugs were found during code review.  Details in the commits.

Please review and apply.

v2:
 - fix up patch 2 properly (Roger Pau Monné)

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>

Amit Shah (2):
  xen: fix out-of-bounds irq unbind for MSI message groups
  xen: events: free irqs in error condition

 drivers/xen/events/events_base.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups
  2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah
@ 2018-02-27 15:55 ` Amit Shah
  2018-02-27 16:58   ` Roger Pau Monné
  2018-02-27 16:58     ` Roger Pau Monné
  2018-02-27 15:55 ` Amit Shah
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 20+ messages in thread
From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw)
  To: xen-devel
  Cc: linux-kernel, Amit Shah, stable, Roger Pau Monné,
	Boris Ostrovsky, Eduardo Valentin, Juergen Gross,
	Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman

When an MSI descriptor was not available, the error path would try to
unbind an irq that was never acquired - potentially unbinding an
unrelated irq.

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1ab4bd1..c86d10e 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 	mutex_unlock(&irq_mapping_update_lock);
 	return irq;
 error_irq:
-	for (; i >= 0; i--)
+	while (i > 0) {
+		i--;
 		__unbind_from_irq(irq + i);
+	}
 	mutex_unlock(&irq_mapping_update_lock);
 	return ret;
 }
-- 
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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups
  2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah
  2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
@ 2018-02-27 15:55 ` Amit Shah
  2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah
  2018-02-27 15:55 ` Amit Shah
  3 siblings, 0 replies; 20+ messages in thread
From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo,
	Boris Ostrovsky, K. Y. Srinivasan, Amit Shah, Thomas Gleixner,
	Anoob Soman, Roger Pau Monné

When an MSI descriptor was not available, the error path would try to
unbind an irq that was never acquired - potentially unbinding an
unrelated irq.

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1ab4bd1..c86d10e 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 	mutex_unlock(&irq_mapping_update_lock);
 	return irq;
 error_irq:
-	for (; i >= 0; i--)
+	while (i > 0) {
+		i--;
 		__unbind_from_irq(irq + i);
+	}
 	mutex_unlock(&irq_mapping_update_lock);
 	return ret;
 }
-- 
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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah
  2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
  2018-02-27 15:55 ` Amit Shah
@ 2018-02-27 15:55 ` Amit Shah
  2018-02-27 17:07   ` Roger Pau Monné
  2018-02-27 17:07     ` Roger Pau Monné
  2018-02-27 15:55 ` Amit Shah
  3 siblings, 2 replies; 20+ messages in thread
From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw)
  To: xen-devel
  Cc: linux-kernel, Amit Shah, stable, Roger Pau Monné,
	Boris Ostrovsky, Eduardo Valentin, Juergen Gross,
	Thomas Gleixner, K. Y. Srinivasan, Liu Shuo, Anoob Soman

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);
-- 
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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah
                   ` (2 preceding siblings ...)
  2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah
@ 2018-02-27 15:55 ` Amit Shah
  3 siblings, 0 replies; 20+ messages in thread
From: Amit Shah @ 2018-02-27 15:55 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo,
	Boris Ostrovsky, K. Y. Srinivasan, Amit Shah, Thomas Gleixner,
	Anoob Soman, Roger Pau Monné

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);
-- 
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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups
  2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
@ 2018-02-27 16:58     ` Roger Pau Monné
  2018-02-27 16:58     ` Roger Pau Monné
  1 sibling, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-27 16:58 UTC (permalink / raw)
  To: Amit Shah
  Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky,
	Eduardo Valentin, Juergen Gross, Thomas Gleixner,
	K. Y. Srinivasan, Liu Shuo, Anoob Soman

On Tue, Feb 27, 2018 at 03:55:57PM +0000, Amit Shah wrote:
> When an MSI descriptor was not available, the error path would try to
> unbind an irq that was never acquired - potentially unbinding an
> unrelated irq.

Those IRQs have been allocated in the xen_allocate_irqs_dynamic call,
so I think the "potentially unbinding an unrelated irq" part is wrong.
The unbind call would be performed against an unbound IRQ, which is
harmless AFAICT.

> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 1ab4bd1..c86d10e 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
>  	mutex_unlock(&irq_mapping_update_lock);
>  	return irq;
>  error_irq:
> -	for (; i >= 0; i--)
> +	while (i > 0) {

while (i--)
	__unbind_from_irq(irq + i);

Although please see reply to patch 2.

Roger.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups
@ 2018-02-27 16:58     ` Roger Pau Monné
  0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-27 16:58 UTC (permalink / raw)
  To: Amit Shah
  Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky,
	Eduardo Valentin, Juergen Gross, Thomas Gleixner,
	K. Y. Srinivasan, Liu Shuo, Anoob Soman

On Tue, Feb 27, 2018 at 03:55:57PM +0000, Amit Shah wrote:
> When an MSI descriptor was not available, the error path would try to
> unbind an irq that was never acquired - potentially unbinding an
> unrelated irq.

Those IRQs have been allocated in the xen_allocate_irqs_dynamic call,
so I think the "potentially unbinding an unrelated irq" part is wrong.
The unbind call would be performed against an unbound IRQ, which is
harmless AFAICT.

> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 1ab4bd1..c86d10e 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
>  	mutex_unlock(&irq_mapping_update_lock);
>  	return irq;
>  error_irq:
> -	for (; i >= 0; i--)
> +	while (i > 0) {

while (i--)
	__unbind_from_irq(irq + i);

Although please see reply to patch 2.

Roger.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups
  2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
@ 2018-02-27 16:58   ` Roger Pau Monné
  2018-02-27 16:58     ` Roger Pau Monné
  1 sibling, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-27 16:58 UTC (permalink / raw)
  To: Amit Shah
  Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo,
	xen-devel, Boris Ostrovsky, K. Y. Srinivasan, Thomas Gleixner,
	Anoob Soman

On Tue, Feb 27, 2018 at 03:55:57PM +0000, Amit Shah wrote:
> When an MSI descriptor was not available, the error path would try to
> unbind an irq that was never acquired - potentially unbinding an
> unrelated irq.

Those IRQs have been allocated in the xen_allocate_irqs_dynamic call,
so I think the "potentially unbinding an unrelated irq" part is wrong.
The unbind call would be performed against an unbound IRQ, which is
harmless AFAICT.

> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 1ab4bd1..c86d10e 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
>  	mutex_unlock(&irq_mapping_update_lock);
>  	return irq;
>  error_irq:
> -	for (; i >= 0; i--)
> +	while (i > 0) {

while (i--)
	__unbind_from_irq(irq + i);

Although please see reply to patch 2.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah
@ 2018-02-27 17:07     ` Roger Pau Monné
  2018-02-27 17:07     ` Roger Pau Monné
  1 sibling, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-27 17:07 UTC (permalink / raw)
  To: Amit Shah
  Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky,
	Eduardo Valentin, Juergen Gross, Thomas Gleixner,
	K. Y. Srinivasan, Liu Shuo, Anoob Soman

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
@ 2018-02-27 17:07     ` Roger Pau Monné
  0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-27 17:07 UTC (permalink / raw)
  To: Amit Shah
  Cc: xen-devel, linux-kernel, stable, Boris Ostrovsky,
	Eduardo Valentin, Juergen Gross, Thomas Gleixner,
	K. Y. Srinivasan, Liu Shuo, Anoob Soman

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah
@ 2018-02-27 17:07   ` Roger Pau Monné
  2018-02-27 17:07     ` Roger Pau Monné
  1 sibling, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-27 17:07 UTC (permalink / raw)
  To: Amit Shah
  Cc: Juergen Gross, Eduardo Valentin, linux-kernel, stable, Liu Shuo,
	xen-devel, Boris Ostrovsky, K. Y. Srinivasan, Thomas Gleixner,
	Anoob Soman

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 17:07     ` Roger Pau Monné
  (?)
  (?)
@ 2018-02-27 17:32     ` Shah, Amit
  2018-02-28  8:16         ` Roger Pau Monné
  2018-02-28  8:16       ` Roger Pau Monné
  -1 siblings, 2 replies; 20+ messages in thread
From: Shah, Amit @ 2018-02-27 17:32 UTC (permalink / raw)
  To: roger.pau
  Cc: boris.ostrovsky, linux-kernel, tglx, kys, Valentin, Eduardo,
	jgross, stable, shuo.a.liu, anoob.soman, xen-devel


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 17:07     ` Roger Pau Monné
  (?)
@ 2018-02-27 17:32     ` Shah, Amit
  -1 siblings, 0 replies; 20+ messages in thread
From: Shah, Amit @ 2018-02-27 17:32 UTC (permalink / raw)
  To: roger.pau
  Cc: jgross, xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu,
	Valentin, Eduardo, boris.ostrovsky, kys, tglx


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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 17:32     ` Shah, Amit
@ 2018-02-28  8:16         ` Roger Pau Monné
  2018-02-28  8:16       ` Roger Pau Monné
  1 sibling, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-28  8:16 UTC (permalink / raw)
  To: Shah, Amit
  Cc: boris.ostrovsky, linux-kernel, tglx, kys, Valentin, Eduardo,
	jgross, stable, shuo.a.liu, anoob.soman, xen-devel

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
@ 2018-02-28  8:16         ` Roger Pau Monné
  0 siblings, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-28  8:16 UTC (permalink / raw)
  To: Shah, Amit
  Cc: boris.ostrovsky, linux-kernel, tglx, kys, Valentin, Eduardo,
	jgross, stable, shuo.a.liu, anoob.soman, xen-devel

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-27 17:32     ` Shah, Amit
  2018-02-28  8:16         ` Roger Pau Monné
@ 2018-02-28  8:16       ` Roger Pau Monné
  1 sibling, 0 replies; 20+ messages in thread
From: Roger Pau Monné @ 2018-02-28  8:16 UTC (permalink / raw)
  To: Shah, Amit
  Cc: jgross, xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu,
	Valentin, Eduardo, boris.ostrovsky, kys, tglx

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-28  8:16         ` Roger Pau Monné
  (?)
  (?)
@ 2018-02-28  8:25         ` Shah, Amit
  2018-02-28  8:27           ` Juergen Gross
  2018-02-28  8:27           ` Juergen Gross
  -1 siblings, 2 replies; 20+ messages in thread
From: Shah, Amit @ 2018-02-28  8:25 UTC (permalink / raw)
  To: roger.pau
  Cc: boris.ostrovsky, linux-kernel, tglx, kys, jgross, stable,
	shuo.a.liu, anoob.soman, Valentin, Eduardo, xen-devel


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-28  8:16         ` Roger Pau Monné
  (?)
@ 2018-02-28  8:25         ` Shah, Amit
  -1 siblings, 0 replies; 20+ messages in thread
From: Shah, Amit @ 2018-02-28  8:25 UTC (permalink / raw)
  To: roger.pau
  Cc: jgross, xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu,
	Valentin, Eduardo, boris.ostrovsky, kys, tglx


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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-28  8:25         ` Shah, Amit
@ 2018-02-28  8:27           ` Juergen Gross
  2018-02-28  8:27           ` Juergen Gross
  1 sibling, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2018-02-28  8:27 UTC (permalink / raw)
  To: Shah, Amit, roger.pau
  Cc: boris.ostrovsky, linux-kernel, tglx, kys, stable, shuo.a.liu,
	anoob.soman, Valentin, Eduardo, xen-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] xen: events: free irqs in error condition
  2018-02-28  8:25         ` Shah, Amit
  2018-02-28  8:27           ` Juergen Gross
@ 2018-02-28  8:27           ` Juergen Gross
  1 sibling, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2018-02-28  8:27 UTC (permalink / raw)
  To: Shah, Amit, roger.pau
  Cc: xen-devel, anoob.soman, linux-kernel, stable, shuo.a.liu,
	Valentin, Eduardo, boris.ostrovsky, kys, tglx

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-02-28  8:27 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-27 15:55 [PATCH v2 0/2] xen: fix bugs in error conditions Amit Shah
2018-02-27 15:55 ` [PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups Amit Shah
2018-02-27 16:58   ` Roger Pau Monné
2018-02-27 16:58   ` Roger Pau Monné
2018-02-27 16:58     ` Roger Pau Monné
2018-02-27 15:55 ` Amit Shah
2018-02-27 15:55 ` [PATCH v2 2/2] xen: events: free irqs in error condition Amit Shah
2018-02-27 17:07   ` Roger Pau Monné
2018-02-27 17:07   ` Roger Pau Monné
2018-02-27 17:07     ` Roger Pau Monné
2018-02-27 17:32     ` Shah, Amit
2018-02-27 17:32     ` Shah, Amit
2018-02-28  8:16       ` Roger Pau Monné
2018-02-28  8:16         ` Roger Pau Monné
2018-02-28  8:25         ` Shah, Amit
2018-02-28  8:25         ` Shah, Amit
2018-02-28  8:27           ` Juergen Gross
2018-02-28  8:27           ` Juergen Gross
2018-02-28  8:16       ` Roger Pau Monné
2018-02-27 15:55 ` Amit Shah

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.