* [PATCH] ocxl: Clarify error path in setup_xsl_irq()
@ 2018-12-10 15:18 Greg Kurz
2018-12-10 17:18 ` Frederic Barrat
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Greg Kurz @ 2018-12-10 15:18 UTC (permalink / raw)
To: linuxppc-dev
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat,
Andrew Donnellan
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 <groug@kaod.org>
---
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)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ocxl: Clarify error path in setup_xsl_irq()
2018-12-10 15:18 [PATCH] ocxl: Clarify error path in setup_xsl_irq() Greg Kurz
@ 2018-12-10 17:18 ` Frederic Barrat
2018-12-11 0:19 ` Andrew Donnellan
2018-12-22 9:54 ` Michael Ellerman
2 siblings, 0 replies; 5+ messages in thread
From: Frederic Barrat @ 2018-12-10 17:18 UTC (permalink / raw)
To: Greg Kurz, linuxppc-dev
Cc: Christophe Lombard, Vaibhav Jain, stable, Frederic Barrat,
Andrew Donnellan
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 <groug@kaod.org>
> ---
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 <fbarrat@linux.ibm.com>
> 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)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ocxl: Clarify error path in setup_xsl_irq()
2018-12-10 15:18 [PATCH] ocxl: Clarify error path in setup_xsl_irq() Greg Kurz
2018-12-10 17:18 ` Frederic Barrat
@ 2018-12-11 0:19 ` Andrew Donnellan
2018-12-20 14:52 ` Greg Kurz
2018-12-22 9:54 ` Michael Ellerman
2 siblings, 1 reply; 5+ messages in thread
From: Andrew Donnellan @ 2018-12-11 0:19 UTC (permalink / raw)
To: Greg Kurz, linuxppc-dev
Cc: stable, Christophe Lombard, Frederic Barrat, Vaibhav Jain
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 <groug@kaod.org>
This is good, thanks.
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> ---
> 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)
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ocxl: Clarify error path in setup_xsl_irq()
2018-12-11 0:19 ` Andrew Donnellan
@ 2018-12-20 14:52 ` Greg Kurz
0 siblings, 0 replies; 5+ messages in thread
From: Greg Kurz @ 2018-12-20 14:52 UTC (permalink / raw)
To: Michael Ellerman
Cc: Frederic Barrat, linuxppc-dev, Christophe Lombard,
Andrew Donnellan, Vaibhav Jain
On Tue, 11 Dec 2018 11:19:55 +1100
Andrew Donnellan <andrew.donnellan@au1.ibm.com> wrote:
> 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 <groug@kaod.org>
>
> This is good, thanks.
>
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
>
Friendly ping before Xmas break :)
> > ---
> > 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)
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ocxl: Clarify error path in setup_xsl_irq()
2018-12-10 15:18 [PATCH] ocxl: Clarify error path in setup_xsl_irq() Greg Kurz
2018-12-10 17:18 ` Frederic Barrat
2018-12-11 0:19 ` Andrew Donnellan
@ 2018-12-22 9:54 ` Michael Ellerman
2 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2018-12-22 9:54 UTC (permalink / raw)
To: Greg Kurz, linuxppc-dev
Cc: Andrew Donnellan, Frederic Barrat, Christophe Lombard, stable,
Vaibhav Jain
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 <groug@kaod.org>
> Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/759bc01586535700fbb1aaf40dd727
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-22 10:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 15:18 [PATCH] ocxl: Clarify error path in setup_xsl_irq() Greg Kurz
2018-12-10 17:18 ` Frederic Barrat
2018-12-11 0:19 ` Andrew Donnellan
2018-12-20 14:52 ` Greg Kurz
2018-12-22 9:54 ` Michael Ellerman
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).