Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
On Fri, 5 Feb 2010 13:51:19 +0800, zhangfei gao wrote: > Thanks for your instruction. > Here is patch to modify some comments of i2c_master_send & > i2c_master_recv, is this OK. > > Thanks > Zhangfei > > From 30fbf1ebf1facba3d280c887e2ecfd0499e7b04b Mon Sep 17 00:00:00 2001 > From: Zhangfei Gao > Date: Sat, 6 Feb 2010 05:38:59 +0800 > Subject: [PATCH] i2c: notes of i2c_master_send & i2c_master_recv > > i2c_master_send & i2c_master_recv not support more than 64bytes > transfer, since msg.len is __u16 type > > Signed-off-by: Zhangfei Gao > --- > Documentation/i2c/writing-clients |3 ++- > drivers/i2c/i2c-core.c|4 ++-- > include/linux/i2c.h |1 + > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/Documentation/i2c/writing-clients > b/Documentation/i2c/writing-clients > index 7860aaf..929a3c3 100644 > --- a/Documentation/i2c/writing-clients > +++ b/Documentation/i2c/writing-clients > @@ -318,7 +318,8 @@ Plain I2C communication > These routines read and write some bytes from/to a client. The client > contains the i2c address, so you do not have to include it. The second > parameter contains the bytes to read/write, the third the number of bytes > -to read/write (must be less than the length of the buffer.) Returned is > +to read/write (must be less than the length of the buffer, also should be > +less than 64K since msg.len is __u16 type.) Returned is > the actual number of bytes read/written. > > int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msg, > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 8d80fce..9607dcc 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -1112,7 +1112,7 @@ EXPORT_SYMBOL(i2c_transfer); > * i2c_master_send - issue a single I2C message in master transmit mode > * @client: Handle to slave device > * @buf: Data that will be written to the slave > - * @count: How many bytes to write > + * @count: How many bytes to write, should be less than 64K since > msg.len is u16 Your patch was line-wrapped by your e-mail client, so it doesn't apply. I've fixed it this time, but please make sure it doesn't happen again for future patches you send. > * > * Returns negative errno, or else the number of bytes written. > */ > @@ -1139,7 +1139,7 @@ EXPORT_SYMBOL(i2c_master_send); > * i2c_master_recv - issue a single I2C message in master receive mode > * @client: Handle to slave device > * @buf: Where to store data read from slave > - * @count: How many bytes to read > + * @count: How many bytes to read, should be less than 64K since msg.len is > u16 > * > * Returns negative errno, or else the number of bytes read. > */ > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 57d41b0..b2dea18 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -53,6 +53,7 @@ struct i2c_board_info; > * on a bus (or read from them). Apart from two basic transfer functions to > * transmit one message at a time, a more complex version can be used to > * transmit an arbitrary number of messages without interruption. > + * Parameter count should be less than 64K since msg.len is __u16 > */ > extern int i2c_master_send(struct i2c_client *client, const char *buf, > int count); Patch applied with minor reformatting, thanks. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
> Hi, Jean > > Thanks for your instruction. > Here is patch to modify some comments of i2c_master_send & > i2c_master_recv, is this OK. > Where's the screaming things, dude? > Thanks > Zhangfei > > From 30fbf1ebf1facba3d280c887e2ecfd0499e7b04b Mon Sep 17 00:00:00 2001 > From: Zhangfei Gao > Date: Sat, 6 Feb 2010 05:38:59 +0800 > Subject: [PATCH] i2c: notes of i2c_master_send & i2c_master_recv > > i2c_master_send & i2c_master_recv not support more than 64bytes > transfer, since msg.len is __u16 type > > Signed-off-by: Zhangfei Gao > --- > Documentation/i2c/writing-clients | 3 ++- > drivers/i2c/i2c-core.c | 4 ++-- > include/linux/i2c.h | 1 + > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/Documentation/i2c/writing-clients > b/Documentation/i2c/writing-clients > index 7860aaf..929a3c3 100644 > --- a/Documentation/i2c/writing-clients > +++ b/Documentation/i2c/writing-clients > @@ -318,7 +318,8 @@ Plain I2C communication > These routines read and write some bytes from/to a client. The client > contains the i2c address, so you do not have to include it. The second > parameter contains the bytes to read/write, the third the number of bytes > -to read/write (must be less than the length of the buffer.) Returned is > +to read/write (must be less than the length of the buffer, also should be > +less than 64K since msg.len is __u16 type.) Returned is > the actual number of bytes read/written. > > int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msg, > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 8d80fce..9607dcc 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -1112,7 +1112,7 @@ EXPORT_SYMBOL(i2c_transfer); > * i2c_master_send - issue a single I2C message in master transmit mode > * @client: Handle to slave device > * @buf: Data that will be written to the slave > - * @count: How many bytes to write > + * @count: How many bytes to write, should be less than 64K since > msg.len is u16 > * > * Returns negative errno, or else the number of bytes written. > */ > @@ -1139,7 +1139,7 @@ EXPORT_SYMBOL(i2c_master_send); > * i2c_master_recv - issue a single I2C message in master receive mode > * @client: Handle to slave device > * @buf: Where to store data read from slave > - * @count: How many bytes to read > + * @count: How many bytes to read, should be less than 64K since msg.len is > u16 > * > * Returns negative errno, or else the number of bytes read. > */ > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 57d41b0..b2dea18 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -53,6 +53,7 @@ struct i2c_board_info; > * on a bus (or read from them). Apart from two basic transfer functions to > * transmit one message at a time, a more complex version can be used to > * transmit an arbitrary number of messages without interruption. > + * Parameter count should be less than 64K since msg.len is __u16 > */ > extern int i2c_master_send(struct i2c_client *client, const char *buf, > int count); > -- > 1.6.0.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
On Thu, Feb 4, 2010 at 9:14 PM, Jean Delvare wrote: > On Thu, 4 Feb 2010 04:47:41 -0800, Eric Miao wrote: >> > How about return error in i2c_master_send & i2c_master_recv when count >> > is bigger than 64K, as suggested by Ben. >> >> I think that's more preferable. Making the count parameter as u16, >> though is going to generate a warning, yet that's usually ignored >> by careless programmer, screaming out when this happens might >> be more useful sometimes. > > Developers ignoring warnings get the pain the deserve. > > A check on "count" would come at the price of a small performance hit > for every caller, even though in 99% of the cases the check isn't > needed. > > That being said I don't care too much and will take whatever patch is > sent to me. > > It would be a good idea to add a note about this limit in > Documentation/i2c/writing-clients and/or include/linux/i2c.h. > >> > The device I used could receive 32K one time instead, the firmware >> > download only takes place on-demand in fact. >> > However, it took some time to debug, since no error info come out. >> > Add error msg may notify users, though transfering more than 64K data >> > one time is rarely happen. > > -- > Jean Delvare > Hi, Jean Thanks for your instruction. Here is patch to modify some comments of i2c_master_send & i2c_master_recv, is this OK. Thanks Zhangfei >From 30fbf1ebf1facba3d280c887e2ecfd0499e7b04b Mon Sep 17 00:00:00 2001 From: Zhangfei Gao Date: Sat, 6 Feb 2010 05:38:59 +0800 Subject: [PATCH] i2c: notes of i2c_master_send & i2c_master_recv i2c_master_send & i2c_master_recv not support more than 64bytes transfer, since msg.len is __u16 type Signed-off-by: Zhangfei Gao --- Documentation/i2c/writing-clients |3 ++- drivers/i2c/i2c-core.c|4 ++-- include/linux/i2c.h |1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Documentation/i2c/writing-clients b/Documentation/i2c/writing-clients index 7860aaf..929a3c3 100644 --- a/Documentation/i2c/writing-clients +++ b/Documentation/i2c/writing-clients @@ -318,7 +318,8 @@ Plain I2C communication These routines read and write some bytes from/to a client. The client contains the i2c address, so you do not have to include it. The second parameter contains the bytes to read/write, the third the number of bytes -to read/write (must be less than the length of the buffer.) Returned is +to read/write (must be less than the length of the buffer, also should be +less than 64K since msg.len is __u16 type.) Returned is the actual number of bytes read/written. int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msg, diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 8d80fce..9607dcc 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -1112,7 +1112,7 @@ EXPORT_SYMBOL(i2c_transfer); * i2c_master_send - issue a single I2C message in master transmit mode * @client: Handle to slave device * @buf: Data that will be written to the slave - * @count: How many bytes to write + * @count: How many bytes to write, should be less than 64K since msg.len is u16 * * Returns negative errno, or else the number of bytes written. */ @@ -1139,7 +1139,7 @@ EXPORT_SYMBOL(i2c_master_send); * i2c_master_recv - issue a single I2C message in master receive mode * @client: Handle to slave device * @buf: Where to store data read from slave - * @count: How many bytes to read + * @count: How many bytes to read, should be less than 64K since msg.len is u16 * * Returns negative errno, or else the number of bytes read. */ diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 57d41b0..b2dea18 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -53,6 +53,7 @@ struct i2c_board_info; * on a bus (or read from them). Apart from two basic transfer functions to * transmit one message at a time, a more complex version can be used to * transmit an arbitrary number of messages without interruption. + * Parameter count should be less than 64K since msg.len is __u16 */ extern int i2c_master_send(struct i2c_client *client, const char *buf, int count); -- 1.6.0.4 0001-i2c-notes-of-i2c_master_send-i2c_master_recv.patch Description: Binary data
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
On Thu, 4 Feb 2010 04:47:41 -0800, Eric Miao wrote: > > How about return error in i2c_master_send & i2c_master_recv when count > > is bigger than 64K, as suggested by Ben. > > I think that's more preferable. Making the count parameter as u16, > though is going to generate a warning, yet that's usually ignored > by careless programmer, screaming out when this happens might > be more useful sometimes. Developers ignoring warnings get the pain the deserve. A check on "count" would come at the price of a small performance hit for every caller, even though in 99% of the cases the check isn't needed. That being said I don't care too much and will take whatever patch is sent to me. It would be a good idea to add a note about this limit in Documentation/i2c/writing-clients and/or include/linux/i2c.h. > > The device I used could receive 32K one time instead, the firmware > > download only takes place on-demand in fact. > > However, it took some time to debug, since no error info come out. > > Add error msg may notify users, though transfering more than 64K data > > one time is rarely happen. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
> How about return error in i2c_master_send & i2c_master_recv when count > is bigger than 64K, as suggested by Ben. I think that's more preferable. Making the count parameter as u16, though is going to generate a warning, yet that's usually ignored by careless programmer, screaming out when this happens might be more useful sometimes. > The device I used could receive 32K one time instead, the firmware > download only takes place on-demand in fact. > However, it took some time to debug, since no error info come out. > Add error msg may notify users, though transfering more than 64K data > one time is rarely happen. > > Thanks > Zhangfei > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
On Thu, Feb 4, 2010 at 5:35 PM, Jean Delvare wrote:
> Hi Gao,
>
> I'm adding the linux-i2c list to Cc.
>
> On Thu, 4 Feb 2010 14:04:03 +0800, zhangfei gao wrote:
>> We found type of i2c_msg.len is __u16, while parameter count in
>> i2c_master_send is int.
>> The mismatch will truncate count from int to u16.
>> For example we downloading firmware which is more than 64K (64K+8) via i2c,
>> i2c would transfer u16 (8 bytes) in fact.
>>
>>
>> From 69ec7599bf0fa28441281be1df76a2f573bb9127 Mon Sep 17 00:00:00 2001
>> From: Zhangfei Gao
>> Date: Fri, 5 Feb 2010 05:12:19 +0800
>> Subject: [PATCH] i2c: i2c_msg.len modify to __u32 type
>>
>> int i2c_master_send(struct i2c_client *client,const char *buf ,int
>> count)
>> {
>> ~
>> msg.len = count;
>> ~
>> }
>> Parameter count would truncate from int to __u16
>>
>> Signed-off-by: Zhangfei Gao
>> ---
>> include/linux/i2c.h | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 57d41b0..4769ce9 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -491,7 +491,7 @@ struct i2c_msg {
>> #define I2C_M_IGNORE_NAK 0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */
>> #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 */
>> + __u32 len; /* msg length */
>> __u8 *buf; /* pointer to msg data */
>> };
>>
>
> Unfortunately struct i2c_msg is part of the user-space ABI so you don't
> get to modify it.
>
> What you can do is add a check on "count" in both i2c_master_send() and
> i2c_master_recv() so that messages which will not fit are rejected
> rather than being silently truncated. Alternatively, the count
> parameters could be turned into u16, so that the limitation is
> immediately visible. This would be more efficient.
>
> I don't think it makes a lot of sense to transfer more than 64 kB at
> once over I2C. I don't know at which speed you operate your bus, but
> even at 400 kHz, it takes 1.5 second to transfer 64 kB. At the regular
> 100 kHz, it takes no less than 6 seconds. This incurs pretty high
> latency over the bus in question... Not to mention the system in its
> entirety if your bus is backed by the never-sleeping i2c-algo-bit or
> similar.
>
> So if the hardware allows it, you should simply split the firmware in
> smaller chunks so that it fits into the current interface.
>
> If this is really impossible, then we'd have to introduce a new
> structure for these long messages. As we can't use these in user-space
> for compatibility reasons, we would have to translate from one
> structure type to the other and back for every user-space transaction.
> Obviously there would be a performance penalty, which is why I'd like
> to avoid it if possible.
>
> --
> Jean Delvare
>
Hi, Jean
Thanks for your reply.
How about return error in i2c_master_send & i2c_master_recv when count
is bigger than 64K, as suggested by Ben.
The device I used could receive 32K one time instead, the firmware
download only takes place on-demand in fact.
However, it took some time to debug, since no error info come out.
Add error msg may notify users, though transfering more than 64K data
one time is rarely happen.
Thanks
Zhangfei
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
Hi Gao,
I'm adding the linux-i2c list to Cc.
On Thu, 4 Feb 2010 14:04:03 +0800, zhangfei gao wrote:
> We found type of i2c_msg.len is __u16, while parameter count in
> i2c_master_send is int.
> The mismatch will truncate count from int to u16.
> For example we downloading firmware which is more than 64K (64K+8) via i2c,
> i2c would transfer u16 (8 bytes) in fact.
>
>
> From 69ec7599bf0fa28441281be1df76a2f573bb9127 Mon Sep 17 00:00:00 2001
> From: Zhangfei Gao
> Date: Fri, 5 Feb 2010 05:12:19 +0800
> Subject: [PATCH] i2c: i2c_msg.len modify to __u32 type
>
> int i2c_master_send(struct i2c_client *client,const char *buf ,int
> count)
> {
> ~
> msg.len = count;
> ~
> }
> Parameter count would truncate from int to __u16
>
> Signed-off-by: Zhangfei Gao
> ---
> include/linux/i2c.h |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 57d41b0..4769ce9 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -491,7 +491,7 @@ struct i2c_msg {
> #define I2C_M_IGNORE_NAK0x1000/* if I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_NO_RD_ACK0x0800/* if I2C_FUNC_PROTOCOL_MANGLING
> */
> #define I2C_M_RECV_LEN0x0400/* length will be first received
> byte */
> -__u16 len;/* msg length*/
> +__u32 len;/* msg length*/
> __u8 *buf;/* pointer to msg data*/
> };
>
Unfortunately struct i2c_msg is part of the user-space ABI so you don't
get to modify it.
What you can do is add a check on "count" in both i2c_master_send() and
i2c_master_recv() so that messages which will not fit are rejected
rather than being silently truncated. Alternatively, the count
parameters could be turned into u16, so that the limitation is
immediately visible. This would be more efficient.
I don't think it makes a lot of sense to transfer more than 64 kB at
once over I2C. I don't know at which speed you operate your bus, but
even at 400 kHz, it takes 1.5 second to transfer 64 kB. At the regular
100 kHz, it takes no less than 6 seconds. This incurs pretty high
latency over the bus in question... Not to mention the system in its
entirety if your bus is backed by the never-sleeping i2c-algo-bit or
similar.
So if the hardware allows it, you should simply split the firmware in
smaller chunks so that it fits into the current interface.
If this is really impossible, then we'd have to introduce a new
structure for these long messages. As we can't use these in user-space
for compatibility reasons, we would have to translate from one
structure type to the other and back for every user-space transaction.
Obviously there would be a performance penalty, which is why I'd like
to avoid it if possible.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fix i2c_msg.len not aligning with i2c_master_send
Hi, Ben
Thanks for your suggestion.
On Thu, Feb 4, 2010 at 3:37 PM, Ben Dooks wrote:
>
> On Thu, Feb 04, 2010 at 02:04:03PM +0800, zhangfei gao wrote:
> > Hi, Jean
> >
> >
> > We found type of i2c_msg.len is __u16, while parameter count in
> > i2c_master_send is int.
> > The mismatch will truncate count from int to u16.
> > For example we downloading firmware which is more than 64K (64K+8) via i2c,
> > i2c would transfer u16 (8 bytes) in fact.
>
> I would be wary of changing the i2c_msg structure as this is exported to
> userspace.
>
>
> I think a better fix would be to test the count parameter to i2c_master_send
> and return something like EMSGSIZE to inform the user it is too long.
>
> If you really want i2c transfers of 64K and above, then you might have to
> add a new struct i2c_msg and update all the callsites to use it. Either that
> or chain a set of msgs together and pass them to the i2c controller in one
> go.
>
> Please discuss on the i2c list.
>
1. Both i2c_master_send and i2c_msg are exported in include/linux/i2c.h.
2. Both i2c_master_send and i2c_master_recv supposed to support int
count, is it suitable to return error when count is bigger than 64K?
3. It may impact a lot if adding new structure :)
include/linux/i2c.h
extern int i2c_master_send(struct i2c_client *client, const char *buf,
int count);
extern int i2c_master_recv(struct i2c_client *client, char *buf, int count);
Thansk
Zhangfei
> > >From 69ec7599bf0fa28441281be1df76a2f573bb9127 Mon Sep 17 00:00:00 2001
> > From: Zhangfei Gao
> > Date: Fri, 5 Feb 2010 05:12:19 +0800
> > Subject: [PATCH] i2c: i2c_msg.len modify to __u32 type
> >
> > int i2c_master_send(struct i2c_client *client,const char *buf ,int
> > count)
> > {
> > ~
> > msg.len = count;
> > ~
> > }
> > Parameter count would truncate from int to __u16
> >
> > Signed-off-by: Zhangfei Gao
> > ---
> > include/linux/i2c.h | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 57d41b0..4769ce9 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -491,7 +491,7 @@ struct i2c_msg {
> > #define I2C_M_IGNORE_NAK 0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */
> > #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 */
> > + __u32 len; /* msg length */
> > __u8 *buf; /* pointer to msg data */
> > };
> >
> > --
> > 1.6.0.4
>
>
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
> --
> --
> Ben
>
> Q: What's a light-year?
> A: One-third less calories than a regular year.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
