[alsa-devel] [PATCH 00/14] parport: check success of attach call
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(-)
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(®istration_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(®istration_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) 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
On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
- 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
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
On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
- 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?
- 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
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:
- 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.
- 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
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:
- 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
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)
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
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
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)
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)
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)
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)
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)
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)
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)
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)
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;
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)
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.
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.
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.
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
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)
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)
participants (6)
-
Dan Carpenter -
Jean Delvare -
Sergei Shtylyov -
Sudip Mukherjee -
Willy Tarreau -
Wolfram Sang