Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-27 Thread Jean Delvare 
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,
-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-27 Thread Al Viro 
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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-27 Thread Al Viro 
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...
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-27 Thread Al Viro 
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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-27 Thread Russell King - ARM Linux 
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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-27 Thread Jean Delvare 
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,
-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-25 Thread Jean Delvare 
Hi Felipe, Shubhrajyoti,

On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
 From: Shubhrajyoti D shubhrajy...@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 shubhrajy...@ti.com
 Signed-off-by: Felipe Balbi ba...@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.)

-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-25 Thread Russell King - ARM Linux 
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 shubhrajy...@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 shubhrajy...@ti.com
  Signed-off-by: Felipe Balbi ba...@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_ACK0x0800  /* 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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-25 Thread Jean Delvare 
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 shubhrajy...@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 shubhrajy...@ti.com
   Signed-off-by: Felipe Balbi ba...@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?

-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-25 Thread Russell King - ARM Linux 
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 shubhrajy...@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 shubhrajy...@ti.com
Signed-off-by: Felipe Balbi ba...@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_ACK0x0800  /* 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?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-25 Thread Jean Delvare 
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.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

2012-10-25 Thread Russell King - ARM Linux 
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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html