[PATCH] ocxl: Clarify error path in setup_xsl_irq()

Implementing rollback with goto and labels is a common practice that
leads to prettier and more maintainable code. FWIW, this design pattern
is already being used in alloc_link() a few lines below in this file.

Do the same in setup_xsl_irq().

Signed-off-by: Greg Kurz <redacted>
---
 drivers/misc/ocxl/link.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index eed92055184d..659977a17405 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c

@@ -273,9 +273,9 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link)
 	spa->irq_name = kasprintf(GFP_KERNEL, "ocxl-xsl-%x-%x-%x",
 				link->domain, link->bus, link->dev);
 	if (!spa->irq_name) {
-		unmap_irq_registers(spa);
 		dev_err(&dev->dev, "Can't allocate name for xsl interrupt\n");
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto err_xsl;
 	}
 	/*
 	 * At some point, we'll need to look into allowing a higher

@@ -283,11 +283,10 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link)
 	 */
 	spa->virq = irq_create_mapping(NULL, hwirq);
 	if (!spa->virq) {
-		kfree(spa->irq_name);
-		unmap_irq_registers(spa);
 		dev_err(&dev->dev,
 			"irq_create_mapping failed for translation interrupt\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto err_name;
 	}
 
 	dev_dbg(&dev->dev, "hwirq %d mapped to virq %d\n", hwirq, spa->virq);

@@ -295,15 +294,21 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link)
 	rc = request_irq(spa->virq, xsl_fault_handler, 0, spa->irq_name,
 			link);
 	if (rc) {
-		irq_dispose_mapping(spa->virq);
-		kfree(spa->irq_name);
-		unmap_irq_registers(spa);
 		dev_err(&dev->dev,
 			"request_irq failed for translation interrupt: %d\n",
 			rc);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto err_mapping;
 	}
 	return 0;
+
+err_mapping:
+	irq_dispose_mapping(spa->virq);
+err_name:
+	kfree(spa->irq_name);
+err_xsl:
+	unmap_irq_registers(spa);
+	return rc;
 }
 
 static void release_xsl_irq(struct link *link)

Le 10/12/2018 à 16:18, Greg Kurz a écrit :

Implementing rollback with goto and labels is a common practice that
leads to prettier and more maintainable code. FWIW, this design pattern
is already being used in alloc_link() a few lines below in this file.

Do the same in setup_xsl_irq().

Signed-off-by: Greg Kurz <redacted>
---

This looks good. I don't have a fixed limit when I start using the "goto 
undo" pattern, so it's likely inconsistent in other places as well. 
Truth is I'm not too fussed either way.

Acked-by: Frederic Barrat <redacted>

On 11/12/18 2:18 am, Greg Kurz wrote:

Implementing rollback with goto and labels is a common practice that
leads to prettier and more maintainable code. FWIW, this design pattern
is already being used in alloc_link() a few lines below in this file.

Do the same in setup_xsl_irq().

Signed-off-by: Greg Kurz <redacted>

This is good, thanks.

Acked-by: Andrew Donnellan <redacted>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

On Tue, 11 Dec 2018 11:19:55 +1100
Andrew Donnellan [off-list ref] wrote:

On 11/12/18 2:18 am, Greg Kurz wrote:

quoted

Implementing rollback with goto and labels is a common practice that
leads to prettier and more maintainable code. FWIW, this design pattern
is already being used in alloc_link() a few lines below in this file.

Do the same in setup_xsl_irq().

Signed-off-by: Greg Kurz <redacted>  

This is good, thanks.

Acked-by: Andrew Donnellan <redacted>

Friendly ping before Xmas break :)

quoted

---
  drivers/misc/ocxl/link.c |   23 ++++++++++++++---------
  1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index eed92055184d..659977a17405 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c

@@ -273,9 +273,9 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link)
  	spa->irq_name = kasprintf(GFP_KERNEL, "ocxl-xsl-%x-%x-%x",
  				link->domain, link->bus, link->dev);
  	if (!spa->irq_name) {
-		unmap_irq_registers(spa);
  		dev_err(&dev->dev, "Can't allocate name for xsl interrupt\n");
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto err_xsl;
  	}
  	/*
  	 * At some point, we'll need to look into allowing a higher

@@ -283,11 +283,10 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link)
  	 */
  	spa->virq = irq_create_mapping(NULL, hwirq);
  	if (!spa->virq) {
-		kfree(spa->irq_name);
-		unmap_irq_registers(spa);
  		dev_err(&dev->dev,
  			"irq_create_mapping failed for translation interrupt\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto err_name;
  	}
  
  	dev_dbg(&dev->dev, "hwirq %d mapped to virq %d\n", hwirq, spa->virq);

@@ -295,15 +294,21 @@ static int setup_xsl_irq(struct pci_dev *dev, struct link *link)
  	rc = request_irq(spa->virq, xsl_fault_handler, 0, spa->irq_name,
  			link);
  	if (rc) {
-		irq_dispose_mapping(spa->virq);
-		kfree(spa->irq_name);
-		unmap_irq_registers(spa);
  		dev_err(&dev->dev,
  			"request_irq failed for translation interrupt: %d\n",
  			rc);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto err_mapping;
  	}
  	return 0;
+
+err_mapping:
+	irq_dispose_mapping(spa->virq);
+err_name:
+	kfree(spa->irq_name);
+err_xsl:
+	unmap_irq_registers(spa);
+	return rc;
  }
  
  static void release_xsl_irq(struct link *link)
  

On Mon, 2018-12-10 at 15:18:13 UTC, Greg Kurz wrote:

Implementing rollback with goto and labels is a common practice that
leads to prettier and more maintainable code. FWIW, this design pattern
is already being used in alloc_link() a few lines below in this file.

Do the same in setup_xsl_irq().

Signed-off-by: Greg Kurz <redacted>
Acked-by: Frederic Barrat <redacted>
Acked-by: Andrew Donnellan <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/759bc01586535700fbb1aaf40dd727

cheers