| Message ID | 20150908110549.411e99a8@endymion.delvare |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Sep 08, 2015 at 11:05:49AM +0200, Jean Delvare wrote: > The ioctl is named I2C_RDWR for "I2C read/write". But references to it > were misspelled "rdrw". Fix them. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Cc: Wolfram Sang <wsa@the-dreams.de> Wow! Amazing how long this went unnoticed/unfixed. > -#define I2C_RDRW_IOCTL_MAX_MSGS 42 > +#define I2C_RDWR_IOCTL_MAX_MSGS 42 > +/* Originally defined with a typo, keep if for now for compatibility */ I would drop the 'for now' and keep it forever. A define doesn't hurt and if it increases backwards compatibility, why not? I will also do s/if/it/. No need to resend. > +#define I2C_RDRW_IOCTL_MAX_MSGS I2C_RDWR_IOCTL_MAX_MSGS Thanks!
On Thu, 10 Sep 2015 20:00:12 +0200, Wolfram Sang wrote: > On Tue, Sep 08, 2015 at 11:05:49AM +0200, Jean Delvare wrote: > > The ioctl is named I2C_RDWR for "I2C read/write". But references to it > > were misspelled "rdrw". Fix them. > > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > Wow! Amazing how long this went unnoticed/unfixed. Indeed :/ I don't think this constant was much (or ever) used in user-space before i2ctransfer. That would be why. > > -#define I2C_RDRW_IOCTL_MAX_MSGS 42 > > +#define I2C_RDWR_IOCTL_MAX_MSGS 42 > > +/* Originally defined with a typo, keep if for now for compatibility */ > > I would drop the 'for now' and keep it forever. A define doesn't hurt > and if it increases backwards compatibility, why not? I will also do > s/if/it/. No need to resend. Sorry for the typo and thanks for catching it. "for now" or not is up to you, my idea was that it probably wasn't used in user-space yet so getting rid of it shouldn't hurt, while keeping it would encourage people to use the wrong one instead of the good one. So on second thought a better strategy may be to NOT keep the compatibility in the first place. It's not an ABI change anyway, and it's a minor thing really. Nobody is ever going to hit this limit in practice, and if one does and did not check beforehand, he/she will simply get -EINVAL from ioctl(), which can be reported to the user just the same. IMHO I2C_RDWR_IOCTL_MAX_MSGS should not have been made visible from user-space in the first place. > > +#define I2C_RDRW_IOCTL_MAX_MSGS I2C_RDWR_IOCTL_MAX_MSGS
> > > -#define I2C_RDRW_IOCTL_MAX_MSGS 42 > > > +#define I2C_RDWR_IOCTL_MAX_MSGS 42 > > > +/* Originally defined with a typo, keep if for now for compatibility */ > > > > I would drop the 'for now' and keep it forever. A define doesn't hurt > > and if it increases backwards compatibility, why not? I will also do > > s/if/it/. No need to resend. > > Sorry for the typo and thanks for catching it. "for now" or not is up > to you, my idea was that it probably wasn't used in user-space yet so > getting rid of it shouldn't hurt, while keeping it would encourage > people to use the wrong one instead of the good one. So on second > thought a better strategy may be to NOT keep the compatibility in the > first place. I really don't want to break userspace. And since this #define is in the uapi headers, I rather be safe than sorry. There might be programs using it we don't know about. So: Applied to for-next, thanks! > can be reported to the user just the same. IMHO I2C_RDWR_IOCTL_MAX_MSGS > should not have been made visible from user-space in the first place. Yes.
--- linux-4.2.orig/drivers/i2c/i2c-dev.c 2015-09-08 09:40:43.287812694 +0200 +++ linux-4.2/drivers/i2c/i2c-dev.c 2015-09-08 09:48:55.191927122 +0200 @@ -235,7 +235,7 @@ static int i2cdev_check_addr(struct i2c_ return result; } -static noinline int i2cdev_ioctl_rdrw(struct i2c_client *client, +static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client, unsigned long arg) { struct i2c_rdwr_ioctl_data rdwr_arg; @@ -250,7 +250,7 @@ static noinline int i2cdev_ioctl_rdrw(st /* Put an arbitrary limit on the number of messages that can * be sent at once */ - if (rdwr_arg.nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) + if (rdwr_arg.nmsgs > I2C_RDWR_IOCTL_MAX_MSGS) return -EINVAL; rdwr_pa = memdup_user(rdwr_arg.msgs, @@ -456,7 +456,7 @@ static long i2cdev_ioctl(struct file *fi return put_user(funcs, (unsigned long __user *)arg); case I2C_RDWR: - return i2cdev_ioctl_rdrw(client, arg); + return i2cdev_ioctl_rdwr(client, arg); case I2C_SMBUS: return i2cdev_ioctl_smbus(client, arg); --- linux-4.2.orig/fs/compat_ioctl.c 2015-09-08 09:40:43.287812694 +0200 +++ linux-4.2/fs/compat_ioctl.c 2015-09-08 09:48:55.193927164 +0200 @@ -686,7 +686,7 @@ static int do_i2c_rdwr_ioctl(unsigned in if (get_user(nmsgs, &udata->nmsgs)) return -EFAULT; - if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) + if (nmsgs > I2C_RDWR_IOCTL_MAX_MSGS) return -EINVAL; if (get_user(datap, &udata->msgs)) --- linux-4.2.orig/include/uapi/linux/i2c-dev.h 2015-09-08 09:40:43.287812694 +0200 +++ linux-4.2/include/uapi/linux/i2c-dev.h 2015-09-08 09:48:55.193927164 +0200 @@ -66,7 +66,9 @@ struct i2c_rdwr_ioctl_data { __u32 nmsgs; /* number of i2c_msgs */ }; -#define I2C_RDRW_IOCTL_MAX_MSGS 42 +#define I2C_RDWR_IOCTL_MAX_MSGS 42 +/* Originally defined with a typo, keep if for now for compatibility */ +#define I2C_RDRW_IOCTL_MAX_MSGS I2C_RDWR_IOCTL_MAX_MSGS #endif /* _UAPI_LINUX_I2C_DEV_H */
The ioctl is named I2C_RDWR for "I2C read/write". But references to it were misspelled "rdrw". Fix them. Signed-off-by: Jean Delvare <jdelvare@suse.de> Cc: Wolfram Sang <wsa@the-dreams.de> --- drivers/i2c/i2c-dev.c | 6 +++--- fs/compat_ioctl.c | 2 +- include/uapi/linux/i2c-dev.h | 4 +++- 3 files changed, 7 insertions(+), 5 deletions(-)