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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Apr 2015 11:33:42 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Wolfram Sang <wsa@...-dreams.de>
Cc:	Sudip Mukherjee <sudipm.mukherjee@...il.com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rodolfo Giometti <giometti@...eenne.com>,
	"James E.J. Bottomley" <JBottomley@...allels.com>,
	Mark Brown <broonie@...nel.org>,
	Willy Tarreau <willy@...a-x.org>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
	netdev@...r.kernel.org, linux-scsi@...r.kernel.org,
	linux-spi@...r.kernel.org, devel@...verdev.osuosl.org,
	alsa-devel@...a-project.org
Subject: Re: [PATCH 12/14] i2c-parport: return proper error values from
 attach

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@...torindia.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/