[PATCH 0/6] staging: unisys: visorbus: cleanup gotos in visorbus_main.c

[PATCH 0/6] staging: unisys: visorbus: cleanup gotos in visorbus_main.c

[David Kershner]

This patchset cleans up the gotos found in visorbus_main.c


David Kershner (6):
  staging: unisys: visorbus: fix up gotos in visorbus_init
  staging: unisys: visorbus: Remove gotos in visorbus_match
  staging: unisys: visorbus: git rid of gotos in
    devmajorminor_create_file
  staging: unisys: visorbus: git rid of gotos in
    register_devmajorminor_attributes
  staging: unisys: visorbus: Cleanup gotos in visordriver_probe_device
  staging: unisys: visorbus: rename visordriver_probe_device gotos

 drivers/staging/unisys/visorbus/visorbus_main.c | 111 ++++++++++--------------
 1 file changed, 47 insertions(+), 64 deletions(-)

-- 
1.9.1

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

[PATCH 1/6] staging: unisys: visorbus: fix up gotos in visorbus_init

[David Kershner]

This patch fixes the gotos in visorbus_init

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 533bb5b..7de390c 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -1477,24 +1477,24 @@ struct channel_size_info {
 int
 visorbus_init(void)
 {
-	int rc = 0;
+	int err;
 
-	POSTCODE_LINUX_3(DRIVER_ENTRY_PC, rc, POSTCODE_SEVERITY_INFO);
+	POSTCODE_LINUX_3(DRIVER_ENTRY_PC, 0, POSTCODE_SEVERITY_INFO);
 	bus_device_info_init(&clientbus_driverinfo,
 			     "clientbus", "visorbus",
 			     VERSION, NULL);
 
-	rc = create_bus_type();
-	if (rc < 0) {
+	err = create_bus_type();
+	if (err < 0) {
 		POSTCODE_LINUX_2(BUS_CREATE_ENTRY_PC, DIAG_SEVERITY_ERR);
-		goto away;
+		goto error;
 	}
 
 	periodic_dev_workqueue = create_singlethread_workqueue("visorbus_dev");
 	if (!periodic_dev_workqueue) {
 		POSTCODE_LINUX_2(CREATE_WORKQUEUE_PC, DIAG_SEVERITY_ERR);
-		rc = -ENOMEM;
-		goto away;
+		err = -ENOMEM;
+		goto error;
 	}
 
 	/* This enables us to receive notifications when devices appear for
@@ -1504,13 +1504,11 @@ visorbus_init(void)
 				     &chipset_responders,
 				     &chipset_driverinfo);
 
-	rc = 0;
+	return 0;
 
-away:
-	if (rc)
-		POSTCODE_LINUX_3(CHIPSET_INIT_FAILURE_PC, rc,
-				 POSTCODE_SEVERITY_ERR);
-	return rc;
+error:
+	POSTCODE_LINUX_3(CHIPSET_INIT_FAILURE_PC, err, POSTCODE_SEVERITY_ERR);
+	return err;
 }
 
 void
-- 
1.9.1

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

[PATCH 2/6] staging: unisys: visorbus: Remove gotos in visorbus_match

[David Kershner]

Gotos in visorbus_match are not needed.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 7de390c..e1ec0b8 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -182,7 +182,6 @@ static int
 visorbus_match(struct device *xdev, struct device_driver *xdrv)
 {
 	uuid_le channel_type;
-	int rc = 0;
 	int i;
 	struct visor_device *dev;
 	struct visor_driver *drv;
@@ -190,26 +189,23 @@ visorbus_match(struct device *xdev, struct device_driver *xdrv)
 	dev = to_visor_device(xdev);
 	drv = to_visor_driver(xdrv);
 	channel_type = visorchannel_get_uuid(dev->visorchannel);
-	if (visorbus_forcematch) {
-		rc = 1;
-		goto away;
-	}
-	if (visorbus_forcenomatch)
-		goto away;
 
+	if (visorbus_forcematch)
+		return 1;
+	if (visorbus_forcenomatch)
+		return 0;
 	if (!drv->channel_types)
-		goto away;
+		return 0;
+
 	for (i = 0;
 	     (uuid_le_cmp(drv->channel_types[i].guid, NULL_UUID_LE) != 0) ||
 	     (drv->channel_types[i].name);
 	     i++)
 		if (uuid_le_cmp(drv->channel_types[i].guid,
-				channel_type) == 0) {
-			rc = i + 1;
-			goto away;
-		}
-away:
-	return rc;
+				channel_type) == 0)
+			return i + 1;
+
+	return 0;
 }
 
 /** This is called when device_unregister() is called for the bus device
-- 
1.9.1

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

[PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[David Kershner]

The gotos in devmajorminor_create_file aren't needed, get rid of them.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index e1ec0b8..37a60ec 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -306,21 +306,17 @@ devmajorminor_create_file(struct visor_device *dev, const char *name,
 {
 	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev->devnodes[0]);
 	struct devmajorminor_attribute *myattr = NULL;
-	int x = -1, rc = 0, slot = -1;
+	int x = -1, slot = -1;
 
 	register_devmajorminor_attributes(dev);
 	for (slot = 0; slot < maxdevnodes; slot++)
 		if (!dev->devnodes[slot].attr)
 			break;
-	if (slot == maxdevnodes) {
-		rc = -ENOMEM;
-		goto away;
-	}
+	if (slot == maxdevnodes)
+		return -ENOMEM;
 	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
-	if (!myattr) {
-		rc = -ENOMEM;
-		goto away;
-	}
+	if (!myattr)
+		return -ENOMEM;
 	myattr->show = DEVMAJORMINOR_ATTR;
 	myattr->store = NULL;
 	myattr->slot = slot;
@@ -331,17 +327,12 @@ devmajorminor_create_file(struct visor_device *dev, const char *name,
 	dev->devnodes[slot].minor = minor;
 	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
 	if (x < 0) {
-		rc = x;
-		goto away;
-	}
-	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
-away:
-	if (rc < 0) {
 		kfree(myattr);
-		myattr = NULL;
 		dev->devnodes[slot].attr = NULL;
+		return x;
 	}
-	return rc;
+	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
+	return 0;
 }
 
 static void
-- 
1.9.1

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

[PATCH 4/6] staging: unisys: visorbus: git rid of gotos in register_devmajorminor_attributes

[David Kershner]

Gotos are not needed in register_devmajorminor_attributes, get rid of them.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 37a60ec..26e0374 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -374,22 +374,19 @@ static struct kobj_type devmajorminor_kobj_type = {
 static int
 register_devmajorminor_attributes(struct visor_device *dev)
 {
-	int rc = 0, x = 0;
+	int x;
 
 	if (dev->kobjdevmajorminor.parent)
-		goto away;	/* already registered */
+		return 0;	/* already registered */
 	x = kobject_init_and_add(&dev->kobjdevmajorminor,
 				 &devmajorminor_kobj_type, &dev->device.kobj,
 				 "devmajorminor");
-	if (x < 0) {
-		rc = x;
-		goto away;
-	}
+	if (x < 0)
+		return x;
 
 	kobject_uevent(&dev->kobjdevmajorminor, KOBJ_ADD);
 
-away:
-	return rc;
+	return 0;
 }
 
 static void
-- 
1.9.1

[PATCH 5/6] staging: unisys: visorbus: Cleanup gotos in visordriver_probe_device

[David Kershner]

Visordriver_probe_device gotos were messy, clean them up.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 26e0374..6a228c8 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -751,20 +751,21 @@ visordriver_probe_device(struct device *xdev)
 	wmb();
 	get_device(&dev->device);
 	if (!drv->probe) {
-		up(&dev->visordriver_callback_lock);
 		rc = -ENODEV;
-		goto away;
+		goto err_put_and_up;
 	}
 	rc = drv->probe(dev);
 	if (rc < 0)
-		goto away;
+		goto err_put_and_up;
 
 	fix_vbus_dev_info(dev);
 	up(&dev->visordriver_callback_lock);
+	return 0; /* success: reference kept via unmatched get_device() */
 	rc = 0;
-away:
-	if (rc != 0)
-		put_device(&dev->device);
+
+err_put_and_up:
+	put_device(&dev->device);
+	up(&dev->visordriver_callback_lock);
 	return rc;
 }
 
-- 
1.9.1

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

[PATCH 6/6] staging: unisys: visorbus: rename visordriver_probe_device gotos

[David Kershner]

Away is ambiguous when specifying error vs success. Make return labels
more meaningful by marking them as error paths.

Signed-off-by: David Kershner <david.kershner@unisys.com>
Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
---
 drivers/staging/unisys/visorbus/visorbus_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
index 6a228c8..b611836 100644
--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -978,7 +978,7 @@ create_visor_device(struct visor_device *dev)
 		POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
 		rc = -EINVAL;
-		goto away;
+		goto err_put;
 	}
 
 	/* bus_id must be a unique name with respect to this bus TYPE
@@ -1008,23 +1008,23 @@ create_visor_device(struct visor_device *dev)
 	if (rc < 0) {
 		POSTCODE_LINUX_3(DEVICE_ADD_PC, chipset_bus_no,
 				 DIAG_SEVERITY_ERR);
-		goto away;
+		goto err_put;
 	}
 
 	rc = register_devmajorminor_attributes(dev);
 	if (rc < 0) {
 		POSTCODE_LINUX_3(DEVICE_REGISTER_FAILURE_PC, chipset_dev_no,
 				 DIAG_SEVERITY_ERR);
-		goto away_unregister;
+		goto err_unregister;
 	}
 
 	list_add_tail(&dev->list_all, &list_all_device_instances);
-	return 0;
+	return 0; /* success: reference kept via unmatched get_device() */
 
-away_unregister:
+err_unregister:
 	device_unregister(&dev->device);
 
-away:
+err_put:
 	put_device(&dev->device);
 	return rc;
 }
-- 
1.9.1

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

Re: [PATCH 5/6] staging: unisys: visorbus: Cleanup gotos in visordriver_probe_device

[Jes Sorensen]

David Kershner <david.kershner@unisys.com> writes:
> Visordriver_probe_device gotos were messy, clean them up.
>
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 26e0374..6a228c8 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -751,20 +751,21 @@ visordriver_probe_device(struct device *xdev)
>  	wmb();
>  	get_device(&dev->device);
>  	if (!drv->probe) {
> -		up(&dev->visordriver_callback_lock);
>  		rc = -ENODEV;
> -		goto away;
> +		goto err_put_and_up;
>  	}
>  	rc = drv->probe(dev);
>  	if (rc < 0)
> -		goto away;
> +		goto err_put_and_up;
>  
>  	fix_vbus_dev_info(dev);
>  	up(&dev->visordriver_callback_lock);
> +	return 0; /* success: reference kept via unmatched get_device() */
>  	rc = 0;
> -away:
> -	if (rc != 0)
> -		put_device(&dev->device);
> +
> +err_put_and_up:
> +	put_device(&dev->device);
> +	up(&dev->visordriver_callback_lock);
>  	return rc;
>  }

David,

This doesn't look right - you add a return 0 but then leave the rc = 0
assignment below it, which will never get executed given the goto label
is further down.

Why this obsession with getting rid of the gotos?

Cheers,
Jes

Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[Neil Horman]

On Tue, Mar 08, 2016 at 08:22:45PM -0500, David Kershner wrote:
> The gotos in devmajorminor_create_file aren't needed, get rid of them.
> 
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index e1ec0b8..37a60ec 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -306,21 +306,17 @@ devmajorminor_create_file(struct visor_device *dev, const char *name,
>  {
>  	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev->devnodes[0]);
>  	struct devmajorminor_attribute *myattr = NULL;
> -	int x = -1, rc = 0, slot = -1;
> +	int x = -1, slot = -1;
>  
>  	register_devmajorminor_attributes(dev);
>  	for (slot = 0; slot < maxdevnodes; slot++)
>  		if (!dev->devnodes[slot].attr)
>  			break;
> -	if (slot == maxdevnodes) {
> -		rc = -ENOMEM;
> -		goto away;
> -	}
> +	if (slot == maxdevnodes)
> +		return -ENOMEM;
>  	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
> -	if (!myattr) {
> -		rc = -ENOMEM;
> -		goto away;
> -	}
> +	if (!myattr)
> +		return -ENOMEM;
>  	myattr->show = DEVMAJORMINOR_ATTR;
>  	myattr->store = NULL;
>  	myattr->slot = slot;
> @@ -331,17 +327,12 @@ devmajorminor_create_file(struct visor_device *dev, const char *name,
>  	dev->devnodes[slot].minor = minor;
>  	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
>  	if (x < 0) {
> -		rc = x;
> -		goto away;
> -	}
> -	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> -away:
> -	if (rc < 0) {
>  		kfree(myattr);
> -		myattr = NULL;
>  		dev->devnodes[slot].attr = NULL;
> +		return x;
>  	}
> -	return rc;
> +	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> +	return 0;
>  }
>  
>  static void
> -- 
> 1.9.1
> 

This problem wasn't introduced by this patch, but removing the gotos makes it
harder to fix.  The first thing you do is call
register_devmajorminor_attributes, and if the code fails, you should really
unregister those.  What you really need to do is keep the label and gotos, and
add an unregister call at the same place you use the kfree call

Neil

RE: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[Sell, Timothy C]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@redhat.com]
> Sent: Wednesday, March 09, 2016 11:09 AM
> To: external-ges-unisys@redhat.com
> Cc: gregkh@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> *S-Par-Maintainer
> Subject: Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in
> devmajorminor_create_file
> 
> On Tue, Mar 08, 2016 at 08:22:45PM -0500, David Kershner wrote:
> > The gotos in devmajorminor_create_file aren't needed, get rid of them.
> >
> > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> > ---
> >  drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++-------------
> ----
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> b/drivers/staging/unisys/visorbus/visorbus_main.c
> > index e1ec0b8..37a60ec 100644
> > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > @@ -306,21 +306,17 @@ devmajorminor_create_file(struct visor_device
> *dev, const char *name,
> >  {
> >  	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> >devnodes[0]);
> >  	struct devmajorminor_attribute *myattr = NULL;
> > -	int x = -1, rc = 0, slot = -1;
> > +	int x = -1, slot = -1;
> >
> >  	register_devmajorminor_attributes(dev);
> >  	for (slot = 0; slot < maxdevnodes; slot++)
> >  		if (!dev->devnodes[slot].attr)
> >  			break;
> > -	if (slot == maxdevnodes) {
> > -		rc = -ENOMEM;
> > -		goto away;
> > -	}
> > +	if (slot == maxdevnodes)
> > +		return -ENOMEM;
> >  	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
> > -	if (!myattr) {
> > -		rc = -ENOMEM;
> > -		goto away;
> > -	}
> > +	if (!myattr)
> > +		return -ENOMEM;
> >  	myattr->show = DEVMAJORMINOR_ATTR;
> >  	myattr->store = NULL;
> >  	myattr->slot = slot;
> > @@ -331,17 +327,12 @@ devmajorminor_create_file(struct visor_device
> *dev, const char *name,
> >  	dev->devnodes[slot].minor = minor;
> >  	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
> >  	if (x < 0) {
> > -		rc = x;
> > -		goto away;
> > -	}
> > -	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > -away:
> > -	if (rc < 0) {
> >  		kfree(myattr);
> > -		myattr = NULL;
> >  		dev->devnodes[slot].attr = NULL;
> > +		return x;
> >  	}
> > -	return rc;
> > +	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > +	return 0;
> >  }
> >
> >  static void
> > --
> > 1.9.1
> >
> 
> This problem wasn't introduced by this patch, but removing the gotos makes
> it
> harder to fix.  The first thing you do is call
> register_devmajorminor_attributes, and if the code fails, you should really
> unregister those.  What you really need to do is keep the label and gotos,
> and
> add an unregister call at the same place you use the kfree call
> 
> Neil

As part of our code audit to get this code out of staging,
Dan Carpenter has expressed some criticims regarding our use of gotos,
which included our use of vague label names like "away", and many places
were we have a single exit-point for both success and failure exits.
(I believe Dan has researched this and determined this practice to be
error-prone.)  We have been attempting to clean these up, and as a result
have found some places where it is cleaner to just remove the gotos
altogether.  It looks like this was one of those places.

I see your point about register_devmajorminor_attributes, which is valid,
but we just so-happen to have a subsequent patch that removes all
of the devmajorminor stuff, as we don't really need that.  This was a
result of criticism from Greg about our usages of "raw kobjects".

Tim Sell

Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[Neil Horman]

On Wed, Mar 09, 2016 at 05:10:28PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@redhat.com]
> > Sent: Wednesday, March 09, 2016 11:09 AM
> > To: external-ges-unisys@redhat.com
> > Cc: gregkh@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> > *S-Par-Maintainer
> > Subject: Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in
> > devmajorminor_create_file
> > 
> > On Tue, Mar 08, 2016 at 08:22:45PM -0500, David Kershner wrote:
> > > The gotos in devmajorminor_create_file aren't needed, get rid of them.
> > >
> > > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > > Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> > > ---
> > >  drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++-------------
> > ----
> > >  1 file changed, 8 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> > b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > index e1ec0b8..37a60ec 100644
> > > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > @@ -306,21 +306,17 @@ devmajorminor_create_file(struct visor_device
> > *dev, const char *name,
> > >  {
> > >  	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> > >devnodes[0]);
> > >  	struct devmajorminor_attribute *myattr = NULL;
> > > -	int x = -1, rc = 0, slot = -1;
> > > +	int x = -1, slot = -1;
> > >
> > >  	register_devmajorminor_attributes(dev);
> > >  	for (slot = 0; slot < maxdevnodes; slot++)
> > >  		if (!dev->devnodes[slot].attr)
> > >  			break;
> > > -	if (slot == maxdevnodes) {
> > > -		rc = -ENOMEM;
> > > -		goto away;
> > > -	}
> > > +	if (slot == maxdevnodes)
> > > +		return -ENOMEM;
> > >  	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
> > > -	if (!myattr) {
> > > -		rc = -ENOMEM;
> > > -		goto away;
> > > -	}
> > > +	if (!myattr)
> > > +		return -ENOMEM;
> > >  	myattr->show = DEVMAJORMINOR_ATTR;
> > >  	myattr->store = NULL;
> > >  	myattr->slot = slot;
> > > @@ -331,17 +327,12 @@ devmajorminor_create_file(struct visor_device
> > *dev, const char *name,
> > >  	dev->devnodes[slot].minor = minor;
> > >  	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
> > >  	if (x < 0) {
> > > -		rc = x;
> > > -		goto away;
> > > -	}
> > > -	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > -away:
> > > -	if (rc < 0) {
> > >  		kfree(myattr);
> > > -		myattr = NULL;
> > >  		dev->devnodes[slot].attr = NULL;
> > > +		return x;
> > >  	}
> > > -	return rc;
> > > +	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > +	return 0;
> > >  }
> > >
> > >  static void
> > > --
> > > 1.9.1
> > >
> > 
> > This problem wasn't introduced by this patch, but removing the gotos makes
> > it
> > harder to fix.  The first thing you do is call
> > register_devmajorminor_attributes, and if the code fails, you should really
> > unregister those.  What you really need to do is keep the label and gotos,
> > and
> > add an unregister call at the same place you use the kfree call
> > 
> > Neil
> 
> As part of our code audit to get this code out of staging,
> Dan Carpenter has expressed some criticims regarding our use of gotos,
> which included our use of vague label names like "away", and many places
> were we have a single exit-point for both success and failure exits.
> (I believe Dan has researched this and determined this practice to be
> error-prone.)  We have been attempting to clean these up, and as a result
> have found some places where it is cleaner to just remove the gotos
> altogether.  It looks like this was one of those places.
> 
ok, but I don't think dans criticism of your label names needs to translate into
a wholesale removal of goto statements.  Its very common practice to use (when
done appropriately). In fact, Documentation/CodingStyle has an entire chapter
(chapter 7) on the cetralization of function exit paths implemented with goto,
with very valid rationale, and I think Dan would agree with it.

Neil

> I see your point about register_devmajorminor_attributes, which is valid,
> but we just so-happen to have a subsequent patch that removes all
> of the devmajorminor stuff, as we don't really need that.  This was a
> result of criticism from Greg about our usages of "raw kobjects".
> 
> Tim Sell
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[Sell, Timothy C]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@redhat.com]
> Sent: Wednesday, March 09, 2016 1:48 PM
> To: Sell, Timothy C
> Cc: external-ges-unisys@redhat.com; gregkh@linuxfoundation.org;
> driverdev-devel@linuxdriverproject.org; *S-Par-Maintainer
> Subject: Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in
> devmajorminor_create_file
> 
> On Wed, Mar 09, 2016 at 05:10:28PM +0000, Sell, Timothy C wrote:
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@redhat.com]
> > > Sent: Wednesday, March 09, 2016 11:09 AM
> > > To: external-ges-unisys@redhat.com
> > > Cc: gregkh@linuxfoundation.org; driverdev-
> devel@linuxdriverproject.org;
> > > *S-Par-Maintainer
> > > Subject: Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in
> > > devmajorminor_create_file
> > >
> > > On Tue, Mar 08, 2016 at 08:22:45PM -0500, David Kershner wrote:
> > > > The gotos in devmajorminor_create_file aren't needed, get rid of
> them.
> > > >
> > > > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > > > Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> > > > ---
> > > >  drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++---------
> ----
> > > ----
> > > >  1 file changed, 8 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > > index e1ec0b8..37a60ec 100644
> > > > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > > @@ -306,21 +306,17 @@ devmajorminor_create_file(struct
> visor_device
> > > *dev, const char *name,
> > > >  {
> > > >  	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> > > >devnodes[0]);
> > > >  	struct devmajorminor_attribute *myattr = NULL;
> > > > -	int x = -1, rc = 0, slot = -1;
> > > > +	int x = -1, slot = -1;
> > > >
> > > >  	register_devmajorminor_attributes(dev);
> > > >  	for (slot = 0; slot < maxdevnodes; slot++)
> > > >  		if (!dev->devnodes[slot].attr)
> > > >  			break;
> > > > -	if (slot == maxdevnodes) {
> > > > -		rc = -ENOMEM;
> > > > -		goto away;
> > > > -	}
> > > > +	if (slot == maxdevnodes)
> > > > +		return -ENOMEM;
> > > >  	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
> > > > -	if (!myattr) {
> > > > -		rc = -ENOMEM;
> > > > -		goto away;
> > > > -	}
> > > > +	if (!myattr)
> > > > +		return -ENOMEM;
> > > >  	myattr->show = DEVMAJORMINOR_ATTR;
> > > >  	myattr->store = NULL;
> > > >  	myattr->slot = slot;
> > > > @@ -331,17 +327,12 @@ devmajorminor_create_file(struct
> visor_device
> > > *dev, const char *name,
> > > >  	dev->devnodes[slot].minor = minor;
> > > >  	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
> > > >  	if (x < 0) {
> > > > -		rc = x;
> > > > -		goto away;
> > > > -	}
> > > > -	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > > -away:
> > > > -	if (rc < 0) {
> > > >  		kfree(myattr);
> > > > -		myattr = NULL;
> > > >  		dev->devnodes[slot].attr = NULL;
> > > > +		return x;
> > > >  	}
> > > > -	return rc;
> > > > +	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > > +	return 0;
> > > >  }
> > > >
> > > >  static void
> > > > --
> > > > 1.9.1
> > > >
> > >
> > > This problem wasn't introduced by this patch, but removing the gotos
> makes
> > > it
> > > harder to fix.  The first thing you do is call
> > > register_devmajorminor_attributes, and if the code fails, you should
> really
> > > unregister those.  What you really need to do is keep the label and
> gotos,
> > > and
> > > add an unregister call at the same place you use the kfree call
> > >
> > > Neil
> >
> > As part of our code audit to get this code out of staging,
> > Dan Carpenter has expressed some criticims regarding our use of gotos,
> > which included our use of vague label names like "away", and many places
> > were we have a single exit-point for both success and failure exits.
> > (I believe Dan has researched this and determined this practice to be
> > error-prone.)  We have been attempting to clean these up, and as a result
> > have found some places where it is cleaner to just remove the gotos
> > altogether.  It looks like this was one of those places.
> >
> ok, but I don't think dans criticism of your label names needs to translate
> into
> a wholesale removal of goto statements.  Its very common practice to use
> (when
> done appropriately). In fact, Documentation/CodingStyle has an entire
> chapter
> (chapter 7) on the cetralization of function exit paths implemented with
> goto,
> with very valid rationale, and I think Dan would agree with it.

(deleted external-ges-unisys@redhat.com from the cc, as several of us have
already gotten our hands slapped for mixing dist lists...)

I agree; our goal is NOT to remove gotos/labels.  We are just attempting to
clean up our usages of gotos to be more in-line with kernel conventions.
We have only been removing gotos/labels in cases where they naturally
seem to disappear after we clean things up.
This exercise has already uncovered more than 1 bug, so I think it was useful.
I will refamiliarize myself with Documentation/CodingStyle chapter 7.
Thanks Neil.

Tim Sell

> 
> Neil
> 
> > I see your point about register_devmajorminor_attributes, which is valid,
> > but we just so-happen to have a subsequent patch that removes all
> > of the devmajorminor stuff, as we don't really need that.  This was a
> > result of criticism from Greg about our usages of "raw kobjects".
> >
> > Tim Sell
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: unisys: visorbus: Cleanup gotos in visordriver_probe_device

[Greg KH]

On Tue, Mar 08, 2016 at 08:22:47PM -0500, David Kershner wrote:
> Visordriver_probe_device gotos were messy, clean them up.
> 
> Signed-off-by: David Kershner <david.kershner@unisys.com>
> Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 26e0374..6a228c8 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -751,20 +751,21 @@ visordriver_probe_device(struct device *xdev)
>  	wmb();

I know it's not related to this patch, but why in the world do you have
a wmb() here?  That's really not needed, and is a sign that something is
really wrong here...


>  	get_device(&dev->device);
>  	if (!drv->probe) {
> -		up(&dev->visordriver_callback_lock);
>  		rc = -ENODEV;
> -		goto away;
> +		goto err_put_and_up;
>  	}

Why not just check before calling get_device()?

>  	rc = drv->probe(dev);
>  	if (rc < 0)
> -		goto away;
> +		goto err_put_and_up;

No need for this goto at all, just don't call your fix_vbus_dev_info()
call, and return the value given you, making this function a lot
simpler, with no goto even needed.

what a mess...

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

Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[gregkh]

On Wed, Mar 09, 2016 at 05:10:28PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@redhat.com]
> > Sent: Wednesday, March 09, 2016 11:09 AM
> > To: external-ges-unisys@redhat.com
> > Cc: gregkh@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> > *S-Par-Maintainer
> > Subject: Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in
> > devmajorminor_create_file
> > 
> > On Tue, Mar 08, 2016 at 08:22:45PM -0500, David Kershner wrote:
> > > The gotos in devmajorminor_create_file aren't needed, get rid of them.
> > >
> > > Signed-off-by: David Kershner <david.kershner@unisys.com>
> > > Signed-off-by: Timothy Sell <timothy.sell@unisys.com>
> > > ---
> > >  drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++-------------
> > ----
> > >  1 file changed, 8 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> > b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > index e1ec0b8..37a60ec 100644
> > > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > @@ -306,21 +306,17 @@ devmajorminor_create_file(struct visor_device
> > *dev, const char *name,
> > >  {
> > >  	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> > >devnodes[0]);
> > >  	struct devmajorminor_attribute *myattr = NULL;
> > > -	int x = -1, rc = 0, slot = -1;
> > > +	int x = -1, slot = -1;
> > >
> > >  	register_devmajorminor_attributes(dev);
> > >  	for (slot = 0; slot < maxdevnodes; slot++)
> > >  		if (!dev->devnodes[slot].attr)
> > >  			break;
> > > -	if (slot == maxdevnodes) {
> > > -		rc = -ENOMEM;
> > > -		goto away;
> > > -	}
> > > +	if (slot == maxdevnodes)
> > > +		return -ENOMEM;
> > >  	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
> > > -	if (!myattr) {
> > > -		rc = -ENOMEM;
> > > -		goto away;
> > > -	}
> > > +	if (!myattr)
> > > +		return -ENOMEM;
> > >  	myattr->show = DEVMAJORMINOR_ATTR;
> > >  	myattr->store = NULL;
> > >  	myattr->slot = slot;
> > > @@ -331,17 +327,12 @@ devmajorminor_create_file(struct visor_device
> > *dev, const char *name,
> > >  	dev->devnodes[slot].minor = minor;
> > >  	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
> > >  	if (x < 0) {
> > > -		rc = x;
> > > -		goto away;
> > > -	}
> > > -	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > -away:
> > > -	if (rc < 0) {
> > >  		kfree(myattr);
> > > -		myattr = NULL;
> > >  		dev->devnodes[slot].attr = NULL;
> > > +		return x;
> > >  	}
> > > -	return rc;
> > > +	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > +	return 0;
> > >  }
> > >
> > >  static void
> > > --
> > > 1.9.1
> > >
> > 
> > This problem wasn't introduced by this patch, but removing the gotos makes
> > it
> > harder to fix.  The first thing you do is call
> > register_devmajorminor_attributes, and if the code fails, you should really
> > unregister those.  What you really need to do is keep the label and gotos,
> > and
> > add an unregister call at the same place you use the kfree call
> > 
> > Neil
> 
> As part of our code audit to get this code out of staging,
> Dan Carpenter has expressed some criticims regarding our use of gotos,
> which included our use of vague label names like "away", and many places
> were we have a single exit-point for both success and failure exits.
> (I believe Dan has researched this and determined this practice to be
> error-prone.)  We have been attempting to clean these up, and as a result
> have found some places where it is cleaner to just remove the gotos
> altogether.  It looks like this was one of those places.
> 
> I see your point about register_devmajorminor_attributes, which is valid,
> but we just so-happen to have a subsequent patch that removes all
> of the devmajorminor stuff, as we don't really need that.  This was a
> result of criticism from Greg about our usages of "raw kobjects".

I don't want to take a patch now that makes it harder to fix a real bug
just to make a mythical future patch somehow better/smaller.  Please fix
this issue now.

I'm dropping this series from my queue, please fix up and resend.

thanks,

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