[PATCH 0/7] staging: unisys: goto/rc = -1 cleanup series

[PATCH 0/7] staging: unisys: goto/rc = -1 cleanup series

[Benjamin Romer]

This series cleans up all the places where rc = -1 was being done, either
as initialization, or to return an error value. In some places, it can
just be removed, but other places it was better to cleaning up goto
statements and eliminate the -1 at the same time.

Benjamin Romer (7):
  staging: unisys: fix return value for visorbus pci probe
  staging: unisys: remove goto in get_vbus_header_info
  staging: unisys: cleanup goto in create_visor_device()
  staging: unisys: get rid of goto in create_bus_instance()
  staging: unisys: remove pointless init of rc in
    chipset_device_create()
  staging: unisys: clean up initiate_chipset_device_pause_resume()
  staging: unisys: return meaningful error for visorchipset_init()

 drivers/staging/unisys/visorbus/visorbus_main.c | 96 +++++++++++--------------
 drivers/staging/unisys/visorbus/visorchipset.c  |  2 +-
 2 files changed, 44 insertions(+), 54 deletions(-)

-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/7] staging: unisys: fix return value for visorbus pci probe

[Benjamin Romer]

Instead of returning -1, return -ENODEV when there is no probe function
found for the device.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index a301385..c0badfa 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -771,7 +771,7 @@ visordriver_probe_device(struct device *xdev)
 	get_device(&dev->device);
 	if (!drv->probe) {
 		up(&dev->visordriver_callback_lock);
-		rc = -1;
+		rc = -ENODEV;
 		goto away;
 	}
 	rc = drv->probe(dev);
-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/7] staging: unisys: remove goto in get_vbus_header_info

[Benjamin Romer]

Remove the rc, the = -1, and all the goto mess here and just return
directly with a meaningful error number.

The caller only cares about success/failure right now, that needs to be
addressed in a later patch series.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index c0badfa..0add9ffe 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -1058,23 +1058,21 @@ static int
 get_vbus_header_info(struct visorchannel *chan,
 		     struct spar_vbus_headerinfo *hdr_info)
 {
-	int rc = -1;
-
 	if (!SPAR_VBUS_CHANNEL_OK_CLIENT(visorchannel_get_header(chan)))
-		goto away;
+		return -EINVAL;
+
 	if (visorchannel_read(chan, sizeof(struct channel_header), hdr_info,
 			      sizeof(*hdr_info)) < 0) {
-		goto away;
+		return -EIO;
 	}
 	if (hdr_info->struct_bytes < sizeof(struct spar_vbus_headerinfo))
-		goto away;
+		return -EINVAL;
+
 	if (hdr_info->device_info_struct_bytes <
 	    sizeof(struct ultra_vbus_deviceinfo)) {
-		goto away;
+		return -EINVAL;
 	}
-	rc = 0;
-away:
-	return rc;
+	return 0;
 }
 
 /* Write the contents of <info> to the struct
-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/7] staging: unisys: cleanup goto in create_visor_device()

[Benjamin Romer]

Get rid of the rc = -1 initialization, and remove the goto
mess entirely. Return a meaningful error on failure in the function, or
the rc from a called function if it fails.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 0add9ffe..cb147fa 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -973,7 +973,7 @@ EXPORT_SYMBOL_GPL(visorbus_disable_channel_interrupts);
 static int
 create_visor_device(struct visor_device *dev)
 {
-	int rc = -1;
+	int rc;
 	u32 chipset_bus_no = dev->chipset_bus_no;
 	u32 chipset_dev_no = dev->chipset_dev_no;
 
@@ -995,7 +995,8 @@ create_visor_device(struct visor_device *dev)
 	if (!dev->periodic_work) {
 		POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
-		goto away;
+		put_device(&dev->device);
+		return -EINVAL;
 	}
 
 	/* bus_id must be a unique name with respect to this bus TYPE
@@ -1025,24 +1026,21 @@ create_visor_device(struct visor_device *dev)
 	if (rc < 0) {
 		POSTCODE_LINUX_3(DEVICE_ADD_PC, chipset_bus_no,
 				 DIAG_SEVERITY_ERR);
-		goto away;
+		put_device(&dev->device);
+		return rc;
 	}
 
 	rc = register_devmajorminor_attributes(dev);
 	if (rc < 0) {
 		POSTCODE_LINUX_3(DEVICE_REGISTER_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
-		goto away_register;
+		device_unregister(&dev->device);
+		put_device(&dev->device);
+		return rc;
 	}
 
 	list_add_tail(&dev->list_all, &list_all_device_instances);
 	return 0;
-
-away_register:
-	device_unregister(&dev->device);
-away:
-	put_device(&dev->device);
-	return rc;
 }
 
 static void
-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/7] staging: unisys: get rid of goto in create_bus_instance()

[Benjamin Romer]

Remove the unnecessary rc and goto messiness, and just handle freeing
the memory before returning an error in the one place where that needs
to happen.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index cb147fa..c9b8637 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -1193,17 +1193,14 @@ fix_vbus_dev_info(struct visor_device *visordev)
 static int
 create_bus_instance(struct visor_device *dev)
 {
-	int rc;
 	int id = dev->chipset_bus_no;
 	struct spar_vbus_headerinfo *hdr_info;
 
 	POSTCODE_LINUX_2(BUS_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
 
 	hdr_info = kzalloc(sizeof(*hdr_info), GFP_KERNEL);
-	if (!hdr_info) {
-		rc = -1;
-		goto away;
-	}
+	if (!hdr_info)
+		return -ENOMEM;
 
 	dev_set_name(&dev->device, "visorbus%d", id);
 	dev->device.bus = &visorbus_type;
@@ -1213,8 +1210,8 @@ create_bus_instance(struct visor_device *dev)
 	if (device_register(&dev->device) < 0) {
 		POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, id,
 				 POSTCODE_SEVERITY_ERR);
-		rc = -1;
-		goto away_mem;
+		kfree(hdr_info);
+		return -ENODEV;
 	}
 
 	if (get_vbus_header_info(dev->visorchannel, hdr_info) >= 0) {
@@ -1230,11 +1227,6 @@ create_bus_instance(struct visor_device *dev)
 	list_add_tail(&dev->list_all, &list_all_bus_instances);
 	dev_set_drvdata(&dev->device, dev);
 	return 0;
-
-away_mem:
-	kfree(hdr_info);
-away:
-	return rc;
 }
 
 /** Remove a device instance for the visor bus itself.
-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/7] staging: unisys: remove pointless init of rc in chipset_device_create()

[Benjamin Romer]

The value of rc is set by calling a function, so there's no need to
initialize it to -1, or anything at all.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index c9b8637..2d3ed4e 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -1316,7 +1316,7 @@ chipset_bus_destroy(struct visor_device *dev)
 static void
 chipset_device_create(struct visor_device *dev_info)
 {
-	int rc = -1;
+	int rc;
 	u32 bus_no = dev_info->chipset_bus_no;
 	u32 dev_no = dev_info->chipset_dev_no;
 
-- 
2.5.0

[PATCH 6/7] staging: unisys: clean up initiate_chipset_device_pause_resume()

[Benjamin Romer]

Simplify this function by removing the goto in favor of handling errors
immediately. Eliminate the vaguely-named variable x, and remove the
unneeded initialization of rc. Lastly, provide meaningful error values
to the callback function instead of just passing back a -1 for failure.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 42 +++++++++++++------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 2d3ed4e..e170e0a 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -1393,7 +1393,7 @@ resume_state_change_complete(struct visor_device *dev, int status)
 static void
 initiate_chipset_device_pause_resume(struct visor_device *dev, bool is_pause)
 {
-	int rc = -1, x;
+	int rc;
 	struct visor_driver *drv = NULL;
 	void (*notify_func)(struct visor_device *dev, int response) = NULL;
 
@@ -1402,14 +1402,18 @@ initiate_chipset_device_pause_resume(struct visor_device *dev, bool is_pause)
 	else
 		notify_func = chipset_responders.device_resume;
 	if (!notify_func)
-		goto away;
+		return;
 
 	drv = to_visor_driver(dev->device.driver);
-	if (!drv)
-		goto away;
+	if (!drv) {
+		(*notify_func)(dev, -ENODEV);
+		return;
+	}
 
-	if (dev->pausing || dev->resuming)
-		goto away;
+	if (dev->pausing || dev->resuming) {
+		(*notify_func)(dev, -EBUSY);
+		return;
+	}
 
 	/* Note that even though both drv->pause() and drv->resume
 	 * specify a callback function, it is NOT necessary for us to
@@ -1419,11 +1423,13 @@ initiate_chipset_device_pause_resume(struct visor_device *dev, bool is_pause)
 	 * visorbus while child function drivers are still running.
 	 */
 	if (is_pause) {
-		if (!drv->pause)
-			goto away;
+		if (!drv->pause) {
+			(*notify_func)(dev, -EINVAL);
+			return;
+		}
 
 		dev->pausing = true;
-		x = drv->pause(dev, pause_state_change_complete);
+		rc = drv->pause(dev, pause_state_change_complete);
 	} else {
 		/* This should be done at BUS resume time, but an
 		 * existing problem prevents us from ever getting a bus
@@ -1432,24 +1438,20 @@ initiate_chipset_device_pause_resume(struct visor_device *dev, bool is_pause)
 		 * would never even get here in that case.
 		 */
 		fix_vbus_dev_info(dev);
-		if (!drv->resume)
-			goto away;
+		if (!drv->resume) {
+			(*notify_func)(dev, -EINVAL);
+			return;
+		}
 
 		dev->resuming = true;
-		x = drv->resume(dev, resume_state_change_complete);
+		rc = drv->resume(dev, resume_state_change_complete);
 	}
-	if (x < 0) {
+	if (rc < 0) {
 		if (is_pause)
 			dev->pausing = false;
 		else
 			dev->resuming = false;
-		goto away;
-	}
-	rc = 0;
-away:
-	if (rc < 0) {
-		if (notify_func)
-			(*notify_func)(dev, rc);
+		(*notify_func)(dev, -EINVAL);
 	}
 }
 
-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 7/7] staging: unisys: return meaningful error for visorchipset_init()

[Benjamin Romer]

The other error paths return meaningful error codes, except for the one
when registering a device, which just returned -1. Let's return ENODEV
when it fails to register instead.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
 drivers/staging/unisys/visorbus/visorchipset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c
index a79aa2d..b75b063 100644
--- a/drivers/staging/unisys/visorbus/visorchipset.c
+++ b/drivers/staging/unisys/visorbus/visorchipset.c
@@ -2316,7 +2316,7 @@ visorchipset_init(struct acpi_device *acpi_device)
 	visorchipset_platform_device.dev.devt = major_dev;
 	if (platform_device_register(&visorchipset_platform_device) < 0) {
 		POSTCODE_LINUX_2(DEVICE_REGISTER_FAILURE_PC, DIAG_SEVERITY_ERR);
-		rc = -1;
+		rc = -ENODEV;
 		goto cleanup;
 	}
 	POSTCODE_LINUX_2(CHIPSET_INIT_SUCCESS_PC, POSTCODE_SEVERITY_INFO);
-- 
2.5.0

Re: [PATCH 3/7] staging: unisys: cleanup goto in create_visor_device()

[Dan Carpenter]

On Tue, Feb 23, 2016 at 10:01:51AM -0500, Benjamin Romer wrote:
> Get rid of the rc = -1 initialization, and remove the goto
> mess entirely. Return a meaningful error on failure in the function, or
> the rc from a called function if it fails.
> 
> Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
> ---

For these kinds of things it's better to use gotos to unwind.  Error
handling should basically be mindless and mechanical.  Name the label
after the thing which is freed or what the label does.

err_unregister:
	device_unregister(&dev->device);
err_put:
	put_device(&dev->device);

	return rc;

When you're reviewing, you only have to care about the most recent
allocation.  After this point we need to call put_device().  After this
point we need to call unregister.  The "goto unregister" is pretty
obvious what it does so it means you don't have to scroll down.  Then
when you read to the bottom, it's pretty obvious that the labels do
what the name implies.

regards,
dan carpenter

Re: [PATCH 3/7] staging: unisys: cleanup goto in create_visor_device()

[Ben Romer]

On Tue, 2016-02-23 at 19:16 +0300, Dan Carpenter wrote:
> When you're reviewing, you only have to care about the most recent
> allocation.  After this point we need to call put_device().  After
> this
> point we need to call unregister.  The "goto unregister" is pretty
> obvious what it does so it means you don't have to scroll down.  Then
> when you read to the bottom, it's pretty obvious that the labels do
> what the name implies.

That makes sense. It seemed to me to be unnecessary complication to use
a goto, since there was only the one place where unregister was needed.

I'll put it back in and resend this one.

-- Ben
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/7] staging: unisys: cleanup goto in create_visor_device()

[Benjamin Romer]

Get rid of the rc = -1 initialization. Return a meaningful error on
failure in the function, or, the rc from a called function if it fails.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>

---

v2: Put the goto back in.
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 0add9ffe..d407f45 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -973,7 +973,7 @@ EXPORT_SYMBOL_GPL(visorbus_disable_channel_interrupts);
 static int
 create_visor_device(struct visor_device *dev)
 {
-	int rc = -1;
+	int rc;
 	u32 chipset_bus_no = dev->chipset_bus_no;
 	u32 chipset_dev_no = dev->chipset_dev_no;
 
@@ -995,6 +995,7 @@ create_visor_device(struct visor_device *dev)
 	if (!dev->periodic_work) {
 		POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
+		rc = -EINVAL;
 		goto away;
 	}
 
@@ -1032,15 +1033,16 @@ create_visor_device(struct visor_device *dev)
 	if (rc < 0) {
 		POSTCODE_LINUX_3(DEVICE_REGISTER_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
-		goto away_register;
+		goto away_unregister;
 	}
 
 	list_add_tail(&dev->list_all, &list_all_device_instances);
 	return 0;
 
-away_register:
+	away_unregister:
 	device_unregister(&dev->device);
-away:
+
+	away:
 	put_device(&dev->device);
 	return rc;
 }
-- 
2.5.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/7] staging: unisys: cleanup goto in create_visor_device()

[Ben Romer]

disregard this patch, I sent the wrong file. v3 coming.

-- Ben

[PATCH 3/7 v3] staging: unisys: cleanup rc -1 in create_visor_device()

[Benjamin Romer]

Get rid of the rc = -1 initialization. Return a meaningful error on
failure in the function, or, the rc from a called function if it fails.

Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>

---

v2: Put the goto back in.
v3: sent the wrong version of the patch by mistake.
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 0add9ffe..f81da4d 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -973,7 +973,7 @@ EXPORT_SYMBOL_GPL(visorbus_disable_channel_interrupts);
 static int
 create_visor_device(struct visor_device *dev)
 {
-	int rc = -1;
+	int rc;
 	u32 chipset_bus_no = dev->chipset_bus_no;
 	u32 chipset_dev_no = dev->chipset_dev_no;
 
@@ -995,6 +995,7 @@ create_visor_device(struct visor_device *dev)
 	if (!dev->periodic_work) {
 		POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
+		rc = -EINVAL;
 		goto away;
 	}
 
@@ -1032,14 +1033,15 @@ create_visor_device(struct visor_device *dev)
 	if (rc < 0) {
 		POSTCODE_LINUX_3(DEVICE_REGISTER_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
-		goto away_register;
+		goto away_unregister;
 	}
 
 	list_add_tail(&dev->list_all, &list_all_device_instances);
 	return 0;
 
-away_register:
+away_unregister:
 	device_unregister(&dev->device);
+
 away:
 	put_device(&dev->device);
 	return rc;
-- 
2.5.0

Re: [PATCH 3/7] staging: unisys: cleanup goto in create_visor_device()

[Dan Carpenter]

On Tue, Feb 23, 2016 at 11:21:14AM -0500, Ben Romer wrote:
> On Tue, 2016-02-23 at 19:16 +0300, Dan Carpenter wrote:
> > When you're reviewing, you only have to care about the most recent
> > allocation.  After this point we need to call put_device().  After
> > this
> > point we need to call unregister.  The "goto unregister" is pretty
> > obvious what it does so it means you don't have to scroll down.  Then
> > when you read to the bottom, it's pretty obvious that the labels do
> > what the name implies.
> 
> That makes sense. It seemed to me to be unnecessary complication to use
> a goto, since there was only the one place where unregister was needed.
> 

Yeah.  But there were three places where put_device() is needed.  In
patch 4 there is only one kfree() on error so that is fine either way.

regards,
dan carpenter

Re: [PATCH v2 3/7] staging: unisys: cleanup goto in create_visor_device()

[Dan Carpenter]

On Tue, Feb 23, 2016 at 12:05:22PM -0500, Benjamin Romer wrote:
> -away_register:
> +	away_unregister:
>  	device_unregister(&dev->device);
> -away:
> +
> +	away:
>  	put_device(&dev->device);

These cause checkpatch.pl errors.  Also "away" is too generic a name.
When I see "goto away;" I have no idea if it does everything or nothing
so I have to scroll down.  If it's call "away_put:" then that's clear to
me without scrolling down.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel