| Message ID | 1350899218-13624-8-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Felipe, Shubhrajyoti, On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote: > From: Shubhrajyoti D <shubhrajyoti@ti.com> > > In case of a NACK, it's wise to tell our clients > drivers about how many bytes were actually transferred. > > Support this by adding an extra field to the struct > i2c_msg which gets incremented the amount of bytes > actually transferred. > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > include/uapi/linux/i2c.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h > index 0e949cb..4b35c9b 100644 > --- a/include/uapi/linux/i2c.h > +++ b/include/uapi/linux/i2c.h > @@ -77,6 +77,7 @@ struct i2c_msg { > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ > __u16 len; /* msg length */ > + __u16 transferred; /* actual bytes transferred */ > __u8 *buf; /* pointer to msg data */ > }; On the principle I am fine with this, however you also need to define who should initialize this field, and to what value. Not all i2c bus drivers will implement this functionality at first. Some may even be unable to ever implement it. As soon as i2c chip drivers start checking this value, you will hit combinations of chip driver checking the value and bus driver not setting it. And it has to work. We have to decide whether it is enough to initialize "transferred" to 0, or if we need a more reliable way to distinguish between "0 bytes transferred" and "the bus driver can't tell". What's your take on this? If we need to distinguish, this could be done using a new I2C_FUNC_* flag, or by initializing "transferred" to (__u16)(-1) instead of 0 for example. Then we have to decide whether initialization is done by the individual drivers, or by i2c-core. By the drivers might be faster, but this may also mean more code (and bugs more likely) than letting i2c-core handle it. The exact balance probably depends on the answer to the previous question (initializing a field to 0 is free in many cases.)
On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote: > Hi Felipe, Shubhrajyoti, > > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote: > > From: Shubhrajyoti D <shubhrajyoti@ti.com> > > > > In case of a NACK, it's wise to tell our clients > > drivers about how many bytes were actually transferred. > > > > Support this by adding an extra field to the struct > > i2c_msg which gets incremented the amount of bytes > > actually transferred. > > > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > --- > > include/uapi/linux/i2c.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h > > index 0e949cb..4b35c9b 100644 > > --- a/include/uapi/linux/i2c.h > > +++ b/include/uapi/linux/i2c.h > > @@ -77,6 +77,7 @@ struct i2c_msg { > > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ > > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ > > __u16 len; /* msg length */ > > + __u16 transferred; /* actual bytes transferred */ > > __u8 *buf; /* pointer to msg data */ > > }; > > On the principle I am fine with this, however you also need to define > who should initialize this field, and to what value. You also miss one very very very big point. This will break every I2C using userspace program out there unless it is rebuilt - this change will require the exact right version of those userspace programs for the kernel that they're being used on. Now that we have the userspace API headers separated, this is now much easier to detect: a patch which touches a uapi header needs much more careful consideration than one which doesn't. So no, strong NAK. This is not how we treat userspace. If we want to change userspace API then we do it in a sane manner, where we provide the new functionality in a way that it doesn't break existing users. There's two ways to do this: 1. Leave the existing struct alone, introduce a new one with new ioctls. Schedule the removal of the old interfaces for maybe 10 years time. 2. Rename the existing struct (eg struct old_i2c_msg), and create a new struct called i2c_msg. Rename the existing ioctls to have OLD_ in their names. Provide the existing ioctls under those names, and make them print a warning once that userspace programs need updating. Schedule the removal of the old interfaces for a shorter number of years than (1); Remember all those "old" syscalls we have... this is no different from those.
On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote: > On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote: > > Hi Felipe, Shubhrajyoti, > > > > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote: > > > From: Shubhrajyoti D <shubhrajyoti@ti.com> > > > > > > In case of a NACK, it's wise to tell our clients > > > drivers about how many bytes were actually transferred. > > > > > > Support this by adding an extra field to the struct > > > i2c_msg which gets incremented the amount of bytes > > > actually transferred. > > > > > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > > --- > > > include/uapi/linux/i2c.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h > > > index 0e949cb..4b35c9b 100644 > > > --- a/include/uapi/linux/i2c.h > > > +++ b/include/uapi/linux/i2c.h > > > @@ -77,6 +77,7 @@ struct i2c_msg { > > > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ > > > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ > > > __u16 len; /* msg length */ > > > + __u16 transferred; /* actual bytes transferred */ > > > __u8 *buf; /* pointer to msg data */ > > > }; > > > > On the principle I am fine with this, however you also need to define > > who should initialize this field, and to what value. > > You also miss one very very very big point. This will break every I2C > using userspace program out there unless it is rebuilt - this change will > require the exact right version of those userspace programs for the > kernel that they're being used on. How that? The extra field is added in a hole, so we don't change the struct size nor the relative positions of existing fields. Why would user-space care?
On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote: > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote: > > On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote: > > > Hi Felipe, Shubhrajyoti, > > > > > > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote: > > > > From: Shubhrajyoti D <shubhrajyoti@ti.com> > > > > > > > > In case of a NACK, it's wise to tell our clients > > > > drivers about how many bytes were actually transferred. > > > > > > > > Support this by adding an extra field to the struct > > > > i2c_msg which gets incremented the amount of bytes > > > > actually transferred. > > > > > > > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > > > --- > > > > include/uapi/linux/i2c.h | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h > > > > index 0e949cb..4b35c9b 100644 > > > > --- a/include/uapi/linux/i2c.h > > > > +++ b/include/uapi/linux/i2c.h > > > > @@ -77,6 +77,7 @@ struct i2c_msg { > > > > #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ > > > > #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ > > > > __u16 len; /* msg length */ > > > > + __u16 transferred; /* actual bytes transferred */ > > > > __u8 *buf; /* pointer to msg data */ > > > > }; > > > > > > On the principle I am fine with this, however you also need to define > > > who should initialize this field, and to what value. > > > > You also miss one very very very big point. This will break every I2C > > using userspace program out there unless it is rebuilt - this change will > > require the exact right version of those userspace programs for the > > kernel that they're being used on. > > How that? The extra field is added in a hole, so we don't change the > struct size nor the relative positions of existing fields. Why would > user-space care? You know the layout of that struct for certain across all Linux supported architectures, including some of the more obscure ones which may not require pointers to be aligned?
On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote: > On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote: > > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote: > > > You also miss one very very very big point. This will break every I2C > > > using userspace program out there unless it is rebuilt - this change will > > > require the exact right version of those userspace programs for the > > > kernel that they're being used on. > > > > How that? The extra field is added in a hole, so we don't change the > > struct size nor the relative positions of existing fields. Why would > > user-space care? > > You know the layout of that struct for certain across all Linux supported > architectures, including some of the more obscure ones which may not > require pointers to be aligned? No I don't, I naively supposed it would be fine. I would expect gcc to always align pointers even when the architecture doesn't need them to be, for performance reasons, even when it doesn't have to, as long as attribute packed isn't used. After all, integers don't _have_ to be aligned on x86, but gcc aligns them. The original idea of using the hole in the i2c_msg structure is from David Brownell, who was apparently familiar with such practice, so I assumed it was OK. Actually I still assume it is, until someone comes with an supported architecture where it is not.
On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote: > On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote: > > On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote: > > > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote: > > > > You also miss one very very very big point. This will break every I2C > > > > using userspace program out there unless it is rebuilt - this change will > > > > require the exact right version of those userspace programs for the > > > > kernel that they're being used on. > > > > > > How that? The extra field is added in a hole, so we don't change the > > > struct size nor the relative positions of existing fields. Why would > > > user-space care? > > > > You know the layout of that struct for certain across all Linux supported > > architectures, including some of the more obscure ones which may not > > require pointers to be aligned? > > No I don't, I naively supposed it would be fine. I would expect gcc to > always align pointers even when the architecture doesn't need them to > be, for performance reasons, even when it doesn't have to, as long as > attribute packed isn't used. After all, integers don't _have_ to be > aligned on x86, but gcc aligns them. > > The original idea of using the hole in the i2c_msg structure is from > David Brownell, who was apparently familiar with such practice, so I > assumed it was OK. Actually I still assume it is, until someone comes > with an supported architecture where it is not. According to Al Viro, that would be m68k.
On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote: > On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote: > > The original idea of using the hole in the i2c_msg structure is from > > David Brownell, who was apparently familiar with such practice, so I > > assumed it was OK. Actually I still assume it is, until someone comes > > with an supported architecture where it is not. > > According to Al Viro, that would be m68k. OK... So to make things clear, let me ask Al directly about it. Al, can you please tell us if: --- a/include/uapi/linux/i2c.h +++ b/include/uapi/linux/i2c.h struct i2c_msg { __u16 addr; /* slave address */ __u16 flags; __u16 len; /* msg length */ + __u16 transferred; /* actual bytes transferred */ __u8 *buf; /* pointer to msg data */ }; would break binary compatibility on m68k or any other architecture you are familiar with? Note that struct i2c_msg isn't declared with attribute packed, so my assumption was that pointer "buf" was align on at least 4 bytes, leaving a hole between len and buf. Am I wrong? Thanks,
On Sat, Oct 27, 2012 at 04:32:24PM +0200, Jean Delvare wrote: > On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote: > > On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote: > > > The original idea of using the hole in the i2c_msg structure is from > > > David Brownell, who was apparently familiar with such practice, so I > > > assumed it was OK. Actually I still assume it is, until someone comes > > > with an supported architecture where it is not. > > > > According to Al Viro, that would be m68k. > > OK... So to make things clear, let me ask Al directly about it. Al, can > you please tell us if: [snip] > would break binary compatibility on m68k or any other architecture you > are familiar with? Note that struct i2c_msg isn't declared with > attribute packed, so my assumption was that pointer "buf" was align on > at least 4 bytes, leaving a hole between len and buf. Am I wrong? You are wrong. Assumption that pointers are aligned to 32bit boundary is simply not true. In particular, on m68k alignment is 16bit, i.e. there struct foo { char x; void *p; }; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes occupied by p. Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be surprised by e.g. coldfire-based SoC with i2c on it.
On Sat, Oct 27, 2012 at 05:40:13PM +0100, Al Viro wrote: > You are wrong. Assumption that pointers are aligned to 32bit boundary > is simply not true. In particular, on m68k alignment is 16bit, i.e. there > struct foo { > char x; > void *p; > }; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes > occupied by p. > > Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be > surprised by e.g. coldfire-based SoC with i2c on it. BTW, that's easily verified - take a cross-compiler and do this: ; cat >a.c <<'EOF' struct { char x; void *y; } v; int z = (char *)&v.y - (char *)&v; EOF ; m68k-linux-gnu-gcc -S a.c ; grep -A1 'z:' a.s z: .long 2 ; and watch what it puts into z. gcc is very liberal about what it considers a constant expression, so it allows that sort of expressions as initializers for global variables. Not a portable C, but convenient for experiments like that; just grab a cross-toolchain and feed it testcases of that kind...
On Sat, Oct 27, 2012 at 06:02:35PM +0100, Al Viro wrote: > On Sat, Oct 27, 2012 at 05:40:13PM +0100, Al Viro wrote: > > > You are wrong. Assumption that pointers are aligned to 32bit boundary > > is simply not true. In particular, on m68k alignment is 16bit, i.e. there > > struct foo { > > char x; > > void *p; > > }; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes > > occupied by p. > > > > Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be > > surprised by e.g. coldfire-based SoC with i2c on it. > > BTW, that's easily verified - take a cross-compiler and do this: > ; cat >a.c <<'EOF' > struct { char x; void *y; } v; > int z = (char *)&v.y - (char *)&v; > EOF > ; m68k-linux-gnu-gcc -S a.c > ; grep -A1 'z:' a.s > z: > .long 2 > ; > and watch what it puts into z. gcc is very liberal about what it considers > a constant expression, so it allows that sort of expressions as initializers > for global variables. Not a portable C, but convenient for experiments like > that; just grab a cross-toolchain and feed it testcases of that kind... ... and google for i2c coldfire immediately turns up this: http://mailman.uclinux.org/pipermail/uclinux-dev/2012-May/051874.html with +config I2C_COLDFIRE + tristate "Freescale Coldfire I2C driver" + depends on !M5272 + help + This driver supports the I2C interface availible on most Freescale + Coldfire processors. + + This driver can be built as a module. If so, the module + will be called i2c-coldfire. in it, along with addition of drivers/i2c/busses/i2c-coldfire.c... IOW, i2c on m68k is quite real. Sorry, no go - you don't even have an excuse of i2c never existing on the architecture in question.
On Sat, Oct 27, 2012 at 04:32:24PM +0200, Jean Delvare wrote: > On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote: > > On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote: > > > The original idea of using the hole in the i2c_msg structure is from > > > David Brownell, who was apparently familiar with such practice, so I > > > assumed it was OK. Actually I still assume it is, until someone comes > > > with an supported architecture where it is not. > > > > According to Al Viro, that would be m68k. > > OK... So to make things clear, let me ask Al directly about it. Al, can > you please tell us if: > > --- a/include/uapi/linux/i2c.h > +++ b/include/uapi/linux/i2c.h > struct i2c_msg { > __u16 addr; /* slave address */ > __u16 flags; > __u16 len; /* msg length */ > + __u16 transferred; /* actual bytes transferred */ > __u8 *buf; /* pointer to msg data */ > }; > > would break binary compatibility on m68k or any other architecture you > are familiar with? Note that struct i2c_msg isn't declared with > attribute packed, so my assumption was that pointer "buf" was align on > at least 4 bytes, leaving a hole between len and buf. Am I wrong? So, you're attitude here is "I don't believe you, you are lying". Well, if you have that level of distrust of your fellow developers, then you don't deserve to be a Linux developer at all - and given that why should I take any notice of you in the future over I2C stuff. Especially as you've just proven that you don't know how to deal properly with APIs... Quite frankly I find your attitude towards me here totally disgusting and insulting. Not surprisingly, I didn't lie (I don't lie) and so I didn't "make up" that M68k is one such architecture, and you've now had the confirmation from Al. So, I look forward to your apology for _implying_ that I'm a liar over this issue, or if not, your resignation as I2C maintainer. Thanks.
Hi Al, On Sat, 27 Oct 2012 18:10:36 +0100, Al Viro wrote: > On Sat, Oct 27, 2012 at 06:02:35PM +0100, Al Viro wrote: > > On Sat, Oct 27, 2012 at 05:40:13PM +0100, Al Viro wrote: > > > > > You are wrong. Assumption that pointers are aligned to 32bit boundary > > > is simply not true. In particular, on m68k alignment is 16bit, i.e. there > > > struct foo { > > > char x; > > > void *p; > > > }; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes > > > occupied by p. > > > > > > Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be > > > surprised by e.g. coldfire-based SoC with i2c on it. > > > > BTW, that's easily verified - take a cross-compiler and do this: > > ; cat >a.c <<'EOF' > > struct { char x; void *y; } v; > > int z = (char *)&v.y - (char *)&v; > > EOF > > ; m68k-linux-gnu-gcc -S a.c > > ; grep -A1 'z:' a.s > > z: > > .long 2 > > ; > > and watch what it puts into z. gcc is very liberal about what it considers > > a constant expression, so it allows that sort of expressions as initializers > > for global variables. Not a portable C, but convenient for experiments like > > that; just grab a cross-toolchain and feed it testcases of that kind... Thanks for the fast and complete answer. > ... and google for i2c coldfire immediately turns up this: > http://mailman.uclinux.org/pipermail/uclinux-dev/2012-May/051874.html > with > +config I2C_COLDFIRE > + tristate "Freescale Coldfire I2C driver" > + depends on !M5272 > + help > + This driver supports the I2C interface availible on most Freescale > + Coldfire processors. > + > + This driver can be built as a module. If so, the module > + will be called i2c-coldfire. > > in it, along with addition of drivers/i2c/busses/i2c-coldfire.c... IOW, > i2c on m68k is quite real. Sorry, no go - you don't even have an excuse > of i2c never existing on the architecture in question. You are completely right, we will have to find another way to let bus drivers report how much of each message they managed to transmit or receive. Thanks again,
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h index 0e949cb..4b35c9b 100644 --- a/include/uapi/linux/i2c.h +++ b/include/uapi/linux/i2c.h @@ -77,6 +77,7 @@ struct i2c_msg { #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ __u16 len; /* msg length */ + __u16 transferred; /* actual bytes transferred */ __u8 *buf; /* pointer to msg data */ };