[PATCH 00/14] parport: check success of attach call

[PATCH 00/14] parport: check success of attach call

[Sudip Mukherjee]

Currently we are not checking if attach has succeeded or not. Also
parport_register_driver() always return 0 even if attach fails.
But in many places where attach has been used, it can fail.
So, modified the parport code to check the return value of attach and
accordingly return either 0 or error code from parport_register_driver.


Sudip Mukherjee (14):
  parport: return value of attach and parport_register_driver
  ALSA: portman2x4: return proper error values from attach
  ALSA: mts64: return proper error values from attach
  staging: panel: return proper error values from attach
  spi: lm70llp: return proper error values from attach
  spi: butterfly: return proper error values from attach
  [SCSI] ppa: return proper error values from attach
  [SCSI] imm: return proper error values from attach
  pps: return proper error values from attach
  pps: return proper error values from attach
  net: plip: return proper error values from attach
  i2c-parport: return proper error values from attach
  ppdev: return proper error values from attach
  char: lp: return proper error values from attach

 drivers/char/lp.c                        | 16 +++++++++++-----
 drivers/char/ppdev.c                     | 10 +++++++---
 drivers/i2c/busses/i2c-parport.c         |  7 ++++---
 drivers/net/plip/plip.c                  | 16 ++++++++++------
 drivers/parport/share.c                  | 20 +++++++++++++++-----
 drivers/pps/clients/pps_parport.c        |  7 ++++---
 drivers/pps/generators/pps_gen_parport.c |  9 +++++----
 drivers/scsi/imm.c                       |  4 ++--
 drivers/scsi/ppa.c                       |  4 ++--
 drivers/spi/spi-butterfly.c              |  7 ++++---
 drivers/spi/spi-lm70llp.c                |  7 ++++---
 drivers/staging/panel/panel.c            | 11 ++++++-----
 include/linux/parport.h                  |  2 +-
 sound/drivers/mts64.c                    | 13 ++++++++-----
 sound/drivers/portman2x4.c               | 15 ++++++++++-----
 15 files changed, 93 insertions(+), 55 deletions(-)

-- 
1.8.1.2

[PATCH 01/14] parport: return value of attach and parport_register_driver

[Sudip Mukherjee]

as of now, we are not checking if attach or parport_register_driver
has succeeded or failed. But attach can fail in the places where they
have been used. Lets check the return of attach, and if attach fails
then parport_register_driver should also fail. We can have multiple
parallel port so we only mark attach as failed only if it has never
returned a 0.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/parport/share.c | 20 +++++++++++++++-----
 include/linux/parport.h |  2 +-
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3fa6624..640ce41 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -148,23 +148,33 @@ static void get_lowlevel_driver (void)
  *	callback, but if the driver wants to take a copy of the
  *	pointer it must call parport_get_port() to do so.
  *
- *	Returns 0 on success.  Currently it always succeeds.
+ *	Returns 0 on success.
  **/
 
 int parport_register_driver (struct parport_driver *drv)
 {
 	struct parport *port;
+	int ret, err;
+	bool attached = false;
 
 	if (list_empty(&portlist))
 		get_lowlevel_driver ();
 
 	mutex_lock(&registration_lock);
-	list_for_each_entry(port, &portlist, list)
-		drv->attach(port);
-	list_add(&drv->list, &drivers);
+	list_for_each_entry(port, &portlist, list) {
+		err = drv->attach(port);
+		if (err == 0)
+			attached = true;
+		else
+			ret = err;
+	}
+	if (attached) {
+		list_add(&drv->list, &drivers);
+		ret = 0;
+	}
 	mutex_unlock(&registration_lock);
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..9411065 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -249,7 +249,7 @@ struct parport {
 
 struct parport_driver {
 	const char *name;
-	void (*attach) (struct parport *);
+	int (*attach)(struct parport *);
 	void (*detach) (struct parport *);
 	struct list_head list;
 };
-- 
1.8.1.2

Re: [PATCH 01/14] parport: return value of attach and parport_register_driver

[Dan Carpenter]

1) We can't apply this patch on its own so this way of breaking up the
patches doesn't work.

2) I was thinking that all the ->attach() calls would have to succeed or
we would bail.  Having some of them succeed and some fail doesn't seem
like it will simplify the driver code very much.  But I can also see
your point.  Hm...

Minor comment:  No need to preserve the error code if there are lots
which we miss.  We may as well hard code an error code.  But that's a
minor thing.  Does this actually simplify the driver code?  That's the
more important thing.

regards,
dan carpenter

Re: [PATCH 01/14] parport: return value of attach and parport_register_driver

[Dan Carpenter]

On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
> 1) We can't apply this patch on its own so this way of breaking up the
> patches doesn't work.
> 

The right thing is to do add an attach_ret().

static int do_attach(drv)
{
	if (drv->attach_ret)
		return drv->attach_ret();
	drv->attach();
	return 0;
}

Then we convert one driver to use the new function pointer and see if
it simplifies the code.  If so we can transition the others as well.  If
not then we give up.

regards,
dan carpenter

Re: [PATCH 01/14] parport: return value of attach and parport_register_driver

[Sudip Mukherjee]

On Wed, Apr 08, 2015 at 02:44:37PM +0300, Dan Carpenter wrote:
> On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
> 
> Then we convert one driver to use the new function pointer and see if
> it simplifies the code.  If so we can transition the others as well.  If
> not then we give up.
i am sending a v2 and also a patch to change one driver.

regards
sudip
> 
> regards,
> dan carpenter
> 

Re: [PATCH 01/14] parport: return value of attach and parport_register_driver

[Sudip Mukherjee]

On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
> 1) We can't apply this patch on its own so this way of breaking up the
> patches doesn't work.
yes, if the first patch is reverted for any reason all the others need
to be reverted also. so then everything in one single patch?
> 
> 2) I was thinking that all the ->attach() calls would have to succeed or
> we would bail.  Having some of them succeed and some fail doesn't seem
> like it will simplify the driver code very much.  But I can also see
> your point.  Hm...
to clarify my point more here: any system might have more than one
parallel port but the module might decide to use just one. so in that
case attach will return 0 for the port that it wishes to use, for others
it will be a error code. So in parport_register_driver if we get error
codes in all the attach calls then we know that attach has definitely
failed, but atleast one 0 means one attach call has succeeded, which
will happen in case of staging/panel, net/plip...

> 
> Minor comment:  No need to preserve the error code if there are lots
> which we miss.  We may as well hard code an error code.  But that's a
> minor thing.  Does this actually simplify the driver code?  That's the
> more important thing.

i don't think this will simplify the driver code, but atleast now
parport_register_driver() will not report success when we have actually
failed. And as a result module_init will also fail which is supposed to
be the actual behviour.

regards
sudip

> 
> regards,
> dan carpenter
> 

Re: [PATCH 01/14] parport: return value of attach and parport_register_driver

[Dan Carpenter]

On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote:
> On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
> > 1) We can't apply this patch on its own so this way of breaking up the
> > patches doesn't work.
> yes, if the first patch is reverted for any reason all the others need
> to be reverted also. so then everything in one single patch?

The problem is that patch 1/1 breaks the build.  The rule is that we
should be able to apply part of a patch series and nothing breaks.  If
we apply the patch series out of order than things break that's our
problem, yes.  But if we apply only 1/1 and it breaks, that's a problem
with the series.

> > 
> > 2) I was thinking that all the ->attach() calls would have to succeed or
> > we would bail.  Having some of them succeed and some fail doesn't seem
> > like it will simplify the driver code very much.  But I can also see
> > your point.  Hm...

My other issue with this patch series which is related to #2 is that
it's not clear that anyone is checking the return value and doing
correct things with it.

Hopefully, when we use the attach_ret() approach then it will be more
clear if/how the return value is useful.

regards,
dan carpenter

Re: [PATCH 01/14] parport: return value of attach and parport_register_driver

[Willy Tarreau]

On Wed, Apr 08, 2015 at 03:27:37PM +0300, Dan Carpenter wrote:
> On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote:
> > On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
> > > 1) We can't apply this patch on its own so this way of breaking up the
> > > patches doesn't work.
> > yes, if the first patch is reverted for any reason all the others need
> > to be reverted also. so then everything in one single patch?
> 
> The problem is that patch 1/1 breaks the build.  The rule is that we
> should be able to apply part of a patch series and nothing breaks.  If
> we apply the patch series out of order than things break that's our
> problem, yes.  But if we apply only 1/1 and it breaks, that's a problem
> with the series.

Yep, keep in mind that "git bisect" will randomly land in the middle of
any set of patches during a debugging session and it could very well land
on this one. If it breaks, that's a real pain for the people trying to
bisect their bug because suddenly they have to deal with a second bug
totally different from theirs.

Regards,
Willy

[PATCH 02/14] ALSA: portman2x4: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 sound/drivers/portman2x4.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
index 464385a..866adbb 100644
--- a/sound/drivers/portman2x4.c
+++ b/sound/drivers/portman2x4.c
@@ -672,32 +672,37 @@ static int snd_portman_probe_port(struct parport *p)
 	return res ? -EIO : 0;
 }
 
-static void snd_portman_attach(struct parport *p)
+static int snd_portman_attach(struct parport *p)
 {
 	struct platform_device *device;
+	int ret;
 
 	device = platform_device_alloc(PLATFORM_DRIVER, device_count);
 	if (!device)
-		return;
+		return -ENOMEM;
 
 	/* Temporary assignment to forward the parport */
 	platform_set_drvdata(device, p);
 
-	if (platform_device_add(device) < 0) {
+	ret = platform_device_add(device);
+
+	if (ret < 0) {
 		platform_device_put(device);
-		return;
+		return ret;
 	}
 
 	/* Since we dont get the return value of probe
 	 * We need to check if device probing succeeded or not */
 	if (!platform_get_drvdata(device)) {
 		platform_device_unregister(device);
-		return;
+		return -ENODEV;
 	}
 
 	/* register device in global table */
 	platform_devices[device_count] = device;
 	device_count++;
+
+	return 0;
 }
 
 static void snd_portman_detach(struct parport *p)
-- 
1.8.1.2

Re: [PATCH 02/14] ALSA: portman2x4: return proper error values from attach

[Sergei Shtylyov]

Hello.

On 4/8/2015 2:20 PM, Sudip Mukherjee wrote:

> now that we are monitoring the return value from attach, make the

    So you've first changed the method prototype and follow up with the 
changes to the actual implementations? That's backward. I'm afraid such 
changes can't be done piecemeal.

> required changes to return proper value from its attach function.

> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>   sound/drivers/portman2x4.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)

> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> index 464385a..866adbb 100644
> --- a/sound/drivers/portman2x4.c
> +++ b/sound/drivers/portman2x4.c
> @@ -672,32 +672,37 @@ static int snd_portman_probe_port(struct parport *p)
>   	return res ? -EIO : 0;
>   }
>
> -static void snd_portman_attach(struct parport *p)
> +static int snd_portman_attach(struct parport *p)
>   {
>   	struct platform_device *device;
> +	int ret;
>
>   	device = platform_device_alloc(PLATFORM_DRIVER, device_count);
>   	if (!device)
> -		return;
> +		return -ENOMEM;
>
>   	/* Temporary assignment to forward the parport */
>   	platform_set_drvdata(device, p);
>
> -	if (platform_device_add(device) < 0) {
> +	ret = platform_device_add(device);
> +

    I don't think empty line is needed here.

> +	if (ret < 0) {
>   		platform_device_put(device);
> -		return;
> +		return ret;
>   	}
>
[...]

WBR, Sergei

Re: [PATCH 02/14] ALSA: portman2x4: return proper error values from attach

[Sudip Mukherjee]

On Wed, Apr 08, 2015 at 04:32:57PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 4/8/2015 2:20 PM, Sudip Mukherjee wrote:
> 
> >now that we are monitoring the return value from attach, make the
> 
>    So you've first changed the method prototype and follow up with
> the changes to the actual implementations? That's backward. I'm
> afraid such changes can't be done piecemeal.
Dan Carpenter has exlained me the problem with this aproach. I have
already sent a v2 which doesnot break anything and if everyone approves
that way then we can change one driver at a time and nothing will
break.

regards
sudip

> 

[PATCH 03/14] ALSA: mts64: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 sound/drivers/mts64.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/drivers/mts64.c b/sound/drivers/mts64.c
index 2a008a9..fb6223e 100644
--- a/sound/drivers/mts64.c
+++ b/sound/drivers/mts64.c
@@ -874,32 +874,35 @@ static int snd_mts64_probe_port(struct parport *p)
 	return res;
 }
 
-static void snd_mts64_attach(struct parport *p)
+static int snd_mts64_attach(struct parport *p)
 {
 	struct platform_device *device;
+	int ret;
 
 	device = platform_device_alloc(PLATFORM_DRIVER, device_count);
 	if (!device)
-		return;
+		return -ENOMEM;
 
 	/* Temporary assignment to forward the parport */
 	platform_set_drvdata(device, p);
 
-	if (platform_device_add(device) < 0) {
+	ret = platform_device_add(device);
+	if (ret < 0) {
 		platform_device_put(device);
-		return;
+		return ret;
 	}
 
 	/* Since we dont get the return value of probe
 	 * We need to check if device probing succeeded or not */
 	if (!platform_get_drvdata(device)) {
 		platform_device_unregister(device);
-		return;
+		return -ENODEV;
 	}
 
 	/* register device in global table */
 	platform_devices[device_count] = device;
 	device_count++;
+	return 0;
 }
 
 static void snd_mts64_detach(struct parport *p)
-- 
1.8.1.2

[PATCH 04/14] staging: panel: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/panel/panel.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ea54fb4..61f6961 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2188,15 +2188,15 @@ static struct notifier_block panel_notifier = {
 	0
 };
 
-static void panel_attach(struct parport *port)
+static int panel_attach(struct parport *port)
 {
 	if (port->number != parport)
-		return;
+		return -ENODEV;
 
 	if (pprt) {
 		pr_err("%s: port->number=%d parport=%d, already registered!\n",
 		       __func__, port->number, parport);
-		return;
+		return -EALREADY;
 	}
 
 	pprt = parport_register_device(port, "panel", NULL, NULL,  /* pf, kf */
@@ -2206,7 +2206,7 @@ static void panel_attach(struct parport *port)
 	if (pprt == NULL) {
 		pr_err("%s: port->number=%d parport=%d, parport_register_device() failed\n",
 		       __func__, port->number, parport);
-		return;
+		return -ENODEV;
 	}
 
 	if (parport_claim(pprt)) {
@@ -2230,7 +2230,7 @@ static void panel_attach(struct parport *port)
 			goto err_lcd_unreg;
 	}
 	register_reboot_notifier(&panel_notifier);
-	return;
+	return 0;
 
 err_lcd_unreg:
 	if (lcd.enabled)
@@ -2238,6 +2238,7 @@ err_lcd_unreg:
 err_unreg_device:
 	parport_unregister_device(pprt);
 	pprt = NULL;
+	return -ENODEV;
 }
 
 static void panel_detach(struct parport *port)
-- 
1.8.1.2

[PATCH 05/14] spi: lm70llp: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/spi/spi-lm70llp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-lm70llp.c b/drivers/spi/spi-lm70llp.c
index ba72347..9b0fde1 100644
--- a/drivers/spi/spi-lm70llp.c
+++ b/drivers/spi/spi-lm70llp.c
@@ -190,7 +190,7 @@ static u32 lm70_txrx(struct spi_device *spi, unsigned nsecs, u32 word, u8 bits)
 	return bitbang_txrx_be_cpha0(spi, nsecs, 0, 0, word, bits);
 }
 
-static void spi_lm70llp_attach(struct parport *p)
+static int spi_lm70llp_attach(struct parport *p)
 {
 	struct pardevice	*pd;
 	struct spi_lm70llp	*pp;
@@ -201,7 +201,7 @@ static void spi_lm70llp_attach(struct parport *p)
 		printk(KERN_WARNING
 			"%s: spi_lm70llp instance already loaded. Aborting.\n",
 			DRVNAME);
-		return;
+		return -EALREADY;
 	}
 
 	/* TODO:  this just _assumes_ a lm70 is there ... no probe;
@@ -281,7 +281,7 @@ static void spi_lm70llp_attach(struct parport *p)
 	pp->spidev_lm70->bits_per_word = 8;
 
 	lm70llp = pp;
-	return;
+	return 0;
 
 out_bitbang_stop:
 	spi_bitbang_stop(&pp->bitbang);
@@ -296,6 +296,7 @@ out_free_master:
 	(void) spi_master_put(master);
 out_fail:
 	pr_info("%s: spi_lm70llp probe fail, status %d\n", DRVNAME, status);
+	return status;
 }
 
 static void spi_lm70llp_detach(struct parport *p)
-- 
1.8.1.2

[PATCH 06/14] spi: butterfly: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/spi/spi-butterfly.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-butterfly.c b/drivers/spi/spi-butterfly.c
index 9a95862..7df16c8 100644
--- a/drivers/spi/spi-butterfly.c
+++ b/drivers/spi/spi-butterfly.c
@@ -190,7 +190,7 @@ static struct flash_platform_data flash = {
 /* REVISIT remove this ugly global and its "only one" limitation */
 static struct butterfly *butterfly;
 
-static void butterfly_attach(struct parport *p)
+static int butterfly_attach(struct parport *p)
 {
 	struct pardevice	*pd;
 	int			status;
@@ -199,7 +199,7 @@ static void butterfly_attach(struct parport *p)
 	struct device		*dev = p->physport->dev;
 
 	if (butterfly || !dev)
-		return;
+		return -ENODEV;
 
 	/* REVISIT:  this just _assumes_ a butterfly is there ... no probe,
 	 * and no way to be selective about what it binds to.
@@ -287,7 +287,7 @@ static void butterfly_attach(struct parport *p)
 
 	pr_info("%s: AVR Butterfly\n", p->name);
 	butterfly = pp;
-	return;
+	return 0;
 
 clean2:
 	/* turn off VCC */
@@ -300,6 +300,7 @@ clean0:
 	(void) spi_master_put(pp->bitbang.master);
 done:
 	pr_debug("%s: butterfly probe, fail %d\n", p->name, status);
+	return status;
 }
 
 static void butterfly_detach(struct parport *p)
-- 
1.8.1.2

[PATCH 07/14] [SCSI] ppa: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/scsi/ppa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ppa.c b/drivers/scsi/ppa.c
index 1db8b26..48898ec 100644
--- a/drivers/scsi/ppa.c
+++ b/drivers/scsi/ppa.c
@@ -1090,9 +1090,9 @@ out:
 	return err;
 }
 
-static void ppa_attach(struct parport *pb)
+static int ppa_attach(struct parport *pb)
 {
-	__ppa_attach(pb);
+	return __ppa_attach(pb);
 }
 
 static void ppa_detach(struct parport *pb)
-- 
1.8.1.2

[PATCH 08/14] [SCSI] imm: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/scsi/imm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/imm.c b/drivers/scsi/imm.c
index 89a8266..e26330a 100644
--- a/drivers/scsi/imm.c
+++ b/drivers/scsi/imm.c
@@ -1225,9 +1225,9 @@ out:
 	return err;
 }
 
-static void imm_attach(struct parport *pb)
+static int imm_attach(struct parport *pb)
 {
-	__imm_attach(pb);
+	return __imm_attach(pb);
 }
 
 static void imm_detach(struct parport *pb)
-- 
1.8.1.2

[PATCH 09/14] pps: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/pps/generators/pps_gen_parport.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
index dcd39fb..499f410 100644
--- a/drivers/pps/generators/pps_gen_parport.c
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -190,18 +190,18 @@ static inline ktime_t next_intr_time(struct pps_generator_pp *dev)
 			dev->port_write_time + 3 * SAFETY_INTERVAL));
 }
 
-static void parport_attach(struct parport *port)
+static int parport_attach(struct parport *port)
 {
 	if (attached) {
 		/* we already have a port */
-		return;
+		return -EALREADY;
 	}
 
 	device.pardev = parport_register_device(port, KBUILD_MODNAME,
 			NULL, NULL, NULL, PARPORT_FLAG_EXCL, &device);
 	if (!device.pardev) {
 		pr_err("couldn't register with %s\n", port->name);
-		return;
+		return -ENODEV;
 	}
 
 	if (parport_claim_or_block(device.pardev) < 0) {
@@ -218,10 +218,11 @@ static void parport_attach(struct parport *port)
 	device.timer.function = hrtimer_event;
 	hrtimer_start(&device.timer, next_intr_time(&device), HRTIMER_MODE_ABS);
 
-	return;
+	return 0;
 
 err_unregister_dev:
 	parport_unregister_device(device.pardev);
+	return -ENODEV;
 }
 
 static void parport_detach(struct parport *port)
-- 
1.8.1.2

[PATCH 10/14] pps: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/pps/clients/pps_parport.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
index 38a8bbe..a411621 100644
--- a/drivers/pps/clients/pps_parport.c
+++ b/drivers/pps/clients/pps_parport.c
@@ -134,7 +134,7 @@ out_both:
 	return;
 }
 
-static void parport_attach(struct parport *port)
+static int parport_attach(struct parport *port)
 {
 	struct pps_client_pp *device;
 	struct pps_source_info info = {
@@ -151,7 +151,7 @@ static void parport_attach(struct parport *port)
 	device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
 	if (!device) {
 		pr_err("memory allocation failed, not attaching\n");
-		return;
+		return -ENOMEM;
 	}
 
 	device->pardev = parport_register_device(port, KBUILD_MODNAME,
@@ -179,7 +179,7 @@ static void parport_attach(struct parport *port)
 
 	pr_info("attached to %s\n", port->name);
 
-	return;
+	return 0;
 
 err_release_dev:
 	parport_release(device->pardev);
@@ -187,6 +187,7 @@ err_unregister_dev:
 	parport_unregister_device(device->pardev);
 err_free:
 	kfree(device);
+	return -ENODEV;
 }
 
 static void parport_detach(struct parport *port)
-- 
1.8.1.2

[PATCH 11/14] net: plip: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
also return the proper error code in module_init.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/net/plip/plip.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c
index 040b897..6706bc3 100644
--- a/drivers/net/plip/plip.c
+++ b/drivers/net/plip/plip.c
@@ -1243,7 +1243,7 @@ plip_searchfor(int list[], int a)
 
 /* plip_attach() is called (by the parport code) when a port is
  * available to use. */
-static void plip_attach (struct parport *port)
+static int plip_attach(struct parport *port)
 {
 	static int unit;
 	struct net_device *dev;
@@ -1254,13 +1254,13 @@ static void plip_attach (struct parport *port)
 	    plip_searchfor(parport, port->number)) {
 		if (unit == PLIP_MAX) {
 			printk(KERN_ERR "plip: too many devices\n");
-			return;
+			return -EINVAL;
 		}
 
 		sprintf(name, "plip%d", unit);
 		dev = alloc_etherdev(sizeof(struct net_local));
 		if (!dev)
-			return;
+			return -ENOMEM;
 
 		strcpy(dev->name, name);
 
@@ -1300,12 +1300,13 @@ static void plip_attach (struct parport *port)
 					 dev->name, dev->base_addr);
 		dev_plip[unit++] = dev;
 	}
-	return;
+	return 0;
 
 err_parport_unregister:
 	parport_unregister_device(nl->pardev);
 err_free_dev:
 	free_netdev(dev);
+	return -ENODEV;
 }
 
 /* plip_detach() is called (by the parport code) when a port is
@@ -1379,6 +1380,8 @@ __setup("plip=", plip_setup);
 
 static int __init plip_init (void)
 {
+	int err;
+
 	if (parport[0] == -2)
 		return 0;
 
@@ -1387,9 +1390,10 @@ static int __init plip_init (void)
 		timid = 0;
 	}
 
-	if (parport_register_driver (&plip_driver)) {
+	err = parport_register_driver(&plip_driver);
+	if (err) {
 		printk (KERN_WARNING "plip: couldn't register driver\n");
-		return 1;
+		return err;
 	}
 
 	return 0;
-- 
1.8.1.2

[PATCH 12/14] i2c-parport: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/i2c/busses/i2c-parport.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a1fac5a..761a775 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -160,14 +160,14 @@ static void i2c_parport_irq(void *data)
 			"SMBus alert received but no ARA client!\n");
 }
 
-static void i2c_parport_attach(struct parport *port)
+static int i2c_parport_attach(struct parport *port)
 {
 	struct i2c_par *adapter;
 
 	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
 	if (adapter == NULL) {
 		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
-		return;
+		return -ENOMEM;
 	}
 
 	pr_debug("i2c-parport: attaching to %s\n", port->name);
@@ -230,13 +230,14 @@ static void i2c_parport_attach(struct parport *port)
 	mutex_lock(&adapter_list_lock);
 	list_add_tail(&adapter->node, &adapter_list);
 	mutex_unlock(&adapter_list_lock);
-	return;
+	return 0;
 
  err_unregister:
 	parport_release(adapter->pdev);
 	parport_unregister_device(adapter->pdev);
  err_free:
 	kfree(adapter);
+	return -ENODEV;
 }
 
 static void i2c_parport_detach(struct parport *port)
-- 
1.8.1.2

Re: [PATCH 12/14] i2c-parport: return proper error values from attach

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On Wed, Apr 08, 2015 at 04:50:38PM +0530, Sudip Mukherjee wrote:
> now that we are monitoring the return value from attach, make the
> required changes to return proper value from its attach function.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/i2c/busses/i2c-parport.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a1fac5a..761a775 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -160,14 +160,14 @@ static void i2c_parport_irq(void *data)
>  			"SMBus alert received but no ARA client!\n");
>  }
>  
> -static void i2c_parport_attach(struct parport *port)
> +static int i2c_parport_attach(struct parport *port)
>  {
>  	struct i2c_par *adapter;
>  
>  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
>  	if (adapter == NULL) {
>  		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
> -		return;
> +		return -ENOMEM;

ENOMEM does not need printout. Please remove printk while we are here.

>  
>  	pr_debug("i2c-parport: attaching to %s\n", port->name);
> @@ -230,13 +230,14 @@ static void i2c_parport_attach(struct parport *port)
>  	mutex_lock(&adapter_list_lock);
>  	list_add_tail(&adapter->node, &adapter_list);
>  	mutex_unlock(&adapter_list_lock);
> -	return;
> +	return 0;
>  
>   err_unregister:
>  	parport_release(adapter->pdev);
>  	parport_unregister_device(adapter->pdev);
>   err_free:
>  	kfree(adapter);
> +	return -ENODEV;

Ideally, we would return different -Esomething for each failing case. We
can leave that for someone who is acutally using the driver. However, I
wonder if ENODEV is a proper catch-all case because the driver core will
not report failures.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 12/14] i2c-parport: return proper error values from attach

[Jean Delvare]

On Thu, 9 Apr 2015 09:13:07 +0200, Wolfram Sang wrote:
> On Wed, Apr 08, 2015 at 04:50:38PM +0530, Sudip Mukherjee wrote:
> > now that we are monitoring the return value from attach, make the
> > required changes to return proper value from its attach function.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> >  drivers/i2c/busses/i2c-parport.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> > index a1fac5a..761a775 100644
> > --- a/drivers/i2c/busses/i2c-parport.c
> > +++ b/drivers/i2c/busses/i2c-parport.c
> > @@ -160,14 +160,14 @@ static void i2c_parport_irq(void *data)
> >  			"SMBus alert received but no ARA client!\n");
> >  }
> >  
> > -static void i2c_parport_attach(struct parport *port)
> > +static int i2c_parport_attach(struct parport *port)
> >  {
> >  	struct i2c_par *adapter;
> >  
> >  	adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
> >  	if (adapter == NULL) {
> >  		printk(KERN_ERR "i2c-parport: Failed to kzalloc\n");
> > -		return;
> > +		return -ENOMEM;
> 
> ENOMEM does not need printout. Please remove printk while we are here.
> 
> >  
> >  	pr_debug("i2c-parport: attaching to %s\n", port->name);
> > @@ -230,13 +230,14 @@ static void i2c_parport_attach(struct parport *port)
> >  	mutex_lock(&adapter_list_lock);
> >  	list_add_tail(&adapter->node, &adapter_list);
> >  	mutex_unlock(&adapter_list_lock);
> > -	return;
> > +	return 0;
> >  
> >   err_unregister:
> >  	parport_release(adapter->pdev);
> >  	parport_unregister_device(adapter->pdev);
> >   err_free:
> >  	kfree(adapter);
> > +	return -ENODEV;
> 
> Ideally, we would return different -Esomething for each failing case. We
> can leave that for someone who is acutally using the driver. However, I
> wonder if ENODEV is a proper catch-all case because the driver core will
> not report failures.

It doesn't really matter that the error codes are different, it matters
that they are meaningful. As much as possible you should pass error
codes from the lower layers. parport_claim_or_block() and
i2c_bit_add_bus() return proper error codes so you should record and
transmit them. Only parport_register_device() does not, so you have to
craft one and -ENODEV seems appropriate to me.

Note: I can test this driver.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 12/14] i2c-parport: return proper error values from attach

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]


> It doesn't really matter that the error codes are different, it matters
> that they are meaningful. As much as possible you should pass error
> codes from the lower layers. parport_claim_or_block() and
> i2c_bit_add_bus() return proper error codes so you should record and
> transmit them.

Oh, surely yes. I assumed they don't and this series is a first step to
fix this. Should have looked myself. Thanks for jumping in here.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 12/14] i2c-parport: return proper error values from attach

[Sudip Mukherjee]

On Thu, Apr 09, 2015 at 12:25:28PM +0200, Wolfram Sang wrote:
> 
> > It doesn't really matter that the error codes are different, it matters
> > that they are meaningful. As much as possible you should pass error
> > codes from the lower layers. parport_claim_or_block() and
> > i2c_bit_add_bus() return proper error codes so you should record and
> > transmit them.
> 
> Oh, surely yes. I assumed they don't and this series is a first step to
> fix this. Should have looked myself. Thanks for jumping in here.
> 
I planned this series to be the first step to fix the attach function
which does not return it succeeded or failed. And if attach has failed
then there is no reason that module_init will report success.

But as Greg pointed out that the first step should be to bring the
parallel port in the driver model. I am working on that now, I will
post a v2 of this series once that modification is done, and that will
need extensive testing.

regards
sudip

[PATCH 13/14] ppdev: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/char/ppdev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index ae0b42b..14374d7 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -748,10 +748,14 @@ static const struct file_operations pp_fops = {
 	.release	= pp_release,
 };
 
-static void pp_attach(struct parport *port)
+static int pp_attach(struct parport *port)
 {
-	device_create(ppdev_class, port->dev, MKDEV(PP_MAJOR, port->number),
-		      NULL, "parport%d", port->number);
+	struct device *dev;
+
+	dev = device_create(ppdev_class, port->dev,
+			    MKDEV(PP_MAJOR, port->number), NULL,
+			    "parport%d", port->number);
+	return PTR_ERR_OR_ZERO(dev);
 }
 
 static void pp_detach(struct parport *port)
-- 
1.8.1.2

[PATCH 14/14] char: lp: return proper error values from attach

[Sudip Mukherjee]

now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/char/lp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index c4094c4..6988480 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -900,34 +900,40 @@ static int lp_register(int nr, struct parport *port)
 	return 0;
 }
 
-static void lp_attach (struct parport *port)
+static int lp_attach(struct parport *port)
 {
 	unsigned int i;
+	int ret = -ENODEV;
 
 	switch (parport_nr[0]) {
 	case LP_PARPORT_UNSPEC:
 	case LP_PARPORT_AUTO:
 		if (parport_nr[0] == LP_PARPORT_AUTO &&
 		    port->probe_info[0].class != PARPORT_CLASS_PRINTER)
-			return;
+			return ret;
 		if (lp_count == LP_NO) {
 			printk(KERN_INFO "lp: ignoring parallel port (max. %d)\n",LP_NO);
-			return;
+			return ret;
 		}
-		if (!lp_register(lp_count, port))
+		if (!lp_register(lp_count, port)) {
 			lp_count++;
+			ret = 0;
+		}
 		break;
 
 	default:
 		for (i = 0; i < LP_NO; i++) {
 			if (port->number == parport_nr[i]) {
-				if (!lp_register(i, port))
+				if (!lp_register(i, port)) {
 					lp_count++;
+					ret = 0;
+				}
 				break;
 			}
 		}
 		break;
 	}
+	return ret;
 }
 
 static void lp_detach (struct parport *port)
-- 
1.8.1.2