i2c_new_device is losing error codes

i2c_new_device is losing error codes

[Jon Smirl]

i2c_new_device() is not propagating error codes up. Callers will need
to be fixed to use PTR_ERR() to recover the errors.

struct i2c_client *
i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
{
	struct i2c_client	*client;
	int			status;

	client = kzalloc(sizeof *client, GFP_KERNEL);
	if (!client)
--should return -ENOMEM;
		return NULL;

	client->adapter = adap;

	client->dev.platform_data = info->platform_data;
	device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);

	client->flags = info->flags & ~I2C_CLIENT_WAKE;
	client->addr = info->addr;
	client->irq = info->irq;

	strlcpy(client->name, info->type, sizeof(client->name));

	/* a new style driver may be bound to this device when we
	 * return from this function, or any later moment (e.g. maybe
	 * hotplugging will load the driver module).  and the device
	 * refcount model is the standard driver model one.
	 */
	status = i2c_attach_client(client);
--error status is not propagated up
	if (status < 0) {
		kfree(client);
		client = NULL;
	}
	return client;
}
EXPORT_SYMBOL_GPL(i2c_new_device);


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: i2c_new_device is losing error codes

[Jean Delvare]

Hi Jon,

On Mon, 21 Jan 2008 19:11:23 -0500, Jon Smirl wrote:
> i2c_new_device() is not propagating error codes up. Callers will need
> to be fixed to use PTR_ERR() to recover the errors.
> 
> struct i2c_client *
> i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> {
> 	struct i2c_client	*client;
> 	int			status;
> 
> 	client = kzalloc(sizeof *client, GFP_KERNEL);
> 	if (!client)
> --should return -ENOMEM;
> 		return NULL;
> 
> 	client->adapter = adap;
> 
> 	client->dev.platform_data = info->platform_data;
> 	device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);
> 
> 	client->flags = info->flags & ~I2C_CLIENT_WAKE;
> 	client->addr = info->addr;
> 	client->irq = info->irq;
> 
> 	strlcpy(client->name, info->type, sizeof(client->name));
> 
> 	/* a new style driver may be bound to this device when we
> 	 * return from this function, or any later moment (e.g. maybe
> 	 * hotplugging will load the driver module).  and the device
> 	 * refcount model is the standard driver model one.
> 	 */
> 	status = i2c_attach_client(client);
> --error status is not propagated up
> 	if (status < 0) {
> 		kfree(client);
> 		client = NULL;
> 	}
> 	return client;
> }
> EXPORT_SYMBOL_GPL(i2c_new_device);

That's an implementation decision. The PTR_ERR() mechanism is not
mandatory to use, I guess we didn't see a benefit in using it for this
function. I can't think of a recoverable error this function could
return, and if all errors are unrecoverable there's little point in
being able to distinguish between then, is there? If you have a use
case where the actual error code returned by i2c_new_device() would
lead the caller to do something different, please present it to us and
we'll discuss it.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: i2c_new_device is losing error codes

[Jon Smirl]

On 1/22/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Jon,
>
> On Mon, 21 Jan 2008 19:11:23 -0500, Jon Smirl wrote:
> > i2c_new_device() is not propagating error codes up. Callers will need
> > to be fixed to use PTR_ERR() to recover the errors.
> >
> > struct i2c_client *
> > i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > {
> >       struct i2c_client       *client;
> >       int                     status;
> >
> >       client = kzalloc(sizeof *client, GFP_KERNEL);
> >       if (!client)
> > --should return -ENOMEM;
> >               return NULL;
> >
> >       client->adapter = adap;
> >
> >       client->dev.platform_data = info->platform_data;
> >       device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);
> >
> >       client->flags = info->flags & ~I2C_CLIENT_WAKE;
> >       client->addr = info->addr;
> >       client->irq = info->irq;
> >
> >       strlcpy(client->name, info->type, sizeof(client->name));
> >
> >       /* a new style driver may be bound to this device when we
> >        * return from this function, or any later moment (e.g. maybe
> >        * hotplugging will load the driver module).  and the device
> >        * refcount model is the standard driver model one.
> >        */
> >       status = i2c_attach_client(client);
> > --error status is not propagated up
> >       if (status < 0) {
> >               kfree(client);
> >               client = NULL;
> >       }
> >       return client;
> > }
> > EXPORT_SYMBOL_GPL(i2c_new_device);
>
> That's an implementation decision. The PTR_ERR() mechanism is not
> mandatory to use, I guess we didn't see a benefit in using it for this
> function. I can't think of a recoverable error this function could
> return, and if all errors are unrecoverable there's little point in
> being able to distinguish between then, is there? If you have a use
> case where the actual error code returned by i2c_new_device() would
> lead the caller to do something different, please present it to us and
> we'll discuss it.

Passing up the actual error code is helpful to humans in locating the
cause of the error. If I have the exact error I can search lower in
the source code for where it was generated. In my code that calls this
function I was going to print out the error result, but it is
pointless if the error is always NULL. Right now if this function
fails I have no clue as to why it failed, I will have to go document
it with printfs and try to reproduce.

>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: i2c_new_device is losing error codes

[Jean Delvare]

On Tue, 22 Jan 2008 10:50:00 -0500, Jon Smirl wrote:
> On 1/22/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > Hi Jon,
> >
> > On Mon, 21 Jan 2008 19:11:23 -0500, Jon Smirl wrote:
> > > i2c_new_device() is not propagating error codes up. Callers will need
> > > to be fixed to use PTR_ERR() to recover the errors.
> > >
> > > struct i2c_client *
> > > i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> > > {
> > >       struct i2c_client       *client;
> > >       int                     status;
> > >
> > >       client = kzalloc(sizeof *client, GFP_KERNEL);
> > >       if (!client)
> > > --should return -ENOMEM;
> > >               return NULL;
> > >
> > >       client->adapter = adap;
> > >
> > >       client->dev.platform_data = info->platform_data;
> > >       device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);
> > >
> > >       client->flags = info->flags & ~I2C_CLIENT_WAKE;
> > >       client->addr = info->addr;
> > >       client->irq = info->irq;
> > >
> > >       strlcpy(client->name, info->type, sizeof(client->name));
> > >
> > >       /* a new style driver may be bound to this device when we
> > >        * return from this function, or any later moment (e.g. maybe
> > >        * hotplugging will load the driver module).  and the device
> > >        * refcount model is the standard driver model one.
> > >        */
> > >       status = i2c_attach_client(client);
> > > --error status is not propagated up
> > >       if (status < 0) {
> > >               kfree(client);
> > >               client = NULL;
> > >       }
> > >       return client;
> > > }
> > > EXPORT_SYMBOL_GPL(i2c_new_device);
> >
> > That's an implementation decision. The PTR_ERR() mechanism is not
> > mandatory to use, I guess we didn't see a benefit in using it for this
> > function. I can't think of a recoverable error this function could
> > return, and if all errors are unrecoverable there's little point in
> > being able to distinguish between then, is there? If you have a use
> > case where the actual error code returned by i2c_new_device() would
> > lead the caller to do something different, please present it to us and
> > we'll discuss it.
> 
> Passing up the actual error code is helpful to humans in locating the
> cause of the error. If I have the exact error I can search lower in
> the source code for where it was generated. In my code that calls this
> function I was going to print out the error result, but it is
> pointless if the error is always NULL. Right now if this function
> fails I have no clue as to why it failed, I will have to go document
> it with printfs and try to reproduce.

If i2c_attach_client() fails, you'll have an error message in the logs.
If i2c_new_device() fails and there is no such log message, then it
means that kzalloc() failed (which I'd say is very unlikely anyway.) So
you can already tell between both cases with the current code.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c