| Message ID | 154445509294.867383.14905585855902294361.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 759bc01586535700fbb1aaf40dd727146e2e8b57 |
| Headers | show |
| Series | ocxl: Clarify error path in setup_xsl_irq() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 49 lines checked |
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) >
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) >
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) > > >
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)
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(-)