[lm-sensors] question about comment in i2c.h

[lm-sensors] question about comment in i2c.h

[Hideki IWAMOTO]

I want to ask about the following comment in i2c.h.
What does "one more for read length in block process call" mean?
In i2c_smbus_block_process_call and i2c_smbus_xfer_emulated,
block[0] is used for the read length.

 union i2c_smbus_data {
         __u8 byte;
         __u16 word;
         __u8 block[I2C_SMBUS_BLOCK_MAX + 3]; /* block[0] is used for length */
                           /* one more for read length in block process call */
                                                     /* and one more for PEC */
 };


----
Hideki IWAMOTO  h-iwamoto@kit.hi-ho.ne.jp

[lm-sensors] question about comment in i2c.h

[Jean Delvare]

Ohaio Hideki,

> I want to ask about the following comment in i2c.h.
> What does "one more for read length in block process call" mean?
> In i2c_smbus_block_process_call and i2c_smbus_xfer_emulated,
> block[0] is used for the read length.

As strange as it may sound considering that this comment has been
around since June 2002, my reading of the code confirms that you are
right and this comment isn't. Digging through the CVS history, I found
that the i2c_smbus_block_process_call used to work differently in its
first incarnation (2002-06-18). It was originally expecing the
underlying implementation (whether hardware or software emulated) to
append the received bytes after the sent ones in the data buffer. This
explains the additional byte required and the comment. It was changed
to the current logic (received bytes overwrite sent bytes at the
beginning of the data buffer) a few weeks after (2002-07-08), but the
changes to union i2c_smbus_data were not reverted.

I fixed this in i2c CVS (kernel/i2c.h) and lm_sensors CVS
(kernel/include/i2c-dev.h), and have prepared a patch for Linux 2.6
which I will send to Greg KH later. You get credits for the discovery,
thanks a lot for reporting.

Arigato,
-- 
Jean Delvare

[lm-sensors] question about comment in i2c.h

[Mark Studebaker]

Your research matches my recollection.
sorry I left the comment in and thanks Hideki and  Khali for researching.
mds


Jean Delvare wrote:
> Ohaio Hideki,
> 
> 
>>I want to ask about the following comment in i2c.h.
>>What does "one more for read length in block process call" mean?
>>In i2c_smbus_block_process_call and i2c_smbus_xfer_emulated,
>>block[0] is used for the read length.
> 
> 
> As strange as it may sound considering that this comment has been
> around since June 2002, my reading of the code confirms that you are
> right and this comment isn't. Digging through the CVS history, I found
> that the i2c_smbus_block_process_call used to work differently in its
> first incarnation (2002-06-18). It was originally expecing the
> underlying implementation (whether hardware or software emulated) to
> append the received bytes after the sent ones in the data buffer. This
> explains the additional byte required and the comment. It was changed
> to the current logic (received bytes overwrite sent bytes at the
> beginning of the data buffer) a few weeks after (2002-07-08), but the
> changes to union i2c_smbus_data were not reverted.
> 
> I fixed this in i2c CVS (kernel/i2c.h) and lm_sensors CVS
> (kernel/include/i2c-dev.h), and have prepared a patch for Linux 2.6
> which I will send to Greg KH later. You get credits for the discovery,
> thanks a lot for reporting.
> 
> Arigato,

[lm-sensors] question about comment in i2c.h

[Mark Studebaker]

One other thing,
the change in 2.4 / CVS probably won't create a binary compatibility problem because
of alignment, but to be safe we may want to change it back. 
2.6 change of course is fine. Comments?


Jean Delvare wrote:
> Ohaio Hideki,
> 
> 
>>I want to ask about the following comment in i2c.h.
>>What does "one more for read length in block process call" mean?
>>In i2c_smbus_block_process_call and i2c_smbus_xfer_emulated,
>>block[0] is used for the read length.
> 
> 
> As strange as it may sound considering that this comment has been
> around since June 2002, my reading of the code confirms that you are
> right and this comment isn't. Digging through the CVS history, I found
> that the i2c_smbus_block_process_call used to work differently in its
> first incarnation (2002-06-18). It was originally expecing the
> underlying implementation (whether hardware or software emulated) to
> append the received bytes after the sent ones in the data buffer. This
> explains the additional byte required and the comment. It was changed
> to the current logic (received bytes overwrite sent bytes at the
> beginning of the data buffer) a few weeks after (2002-07-08), but the
> changes to union i2c_smbus_data were not reverted.
> 
> I fixed this in i2c CVS (kernel/i2c.h) and lm_sensors CVS
> (kernel/include/i2c-dev.h), and have prepared a patch for Linux 2.6
> which I will send to Greg KH later. You get credits for the discovery,
> thanks a lot for reporting.
> 
> Arigato,

[lm-sensors] question about comment in i2c.h

[Jean Delvare]

Hi Mark,

On 2005-09-26, Mark Studebaker wrote:
> the change in 2.4 / CVS probably won't create a binary compatibility
> problem because of alignment, but to be safe we may want to change it
> back. 2.6 change of course is fine. Comments?

I did the change on purpose, exactly for binary compatibility reasons. We
are trying to keep (or restore) binary compatibility between i2c-CVS and
Linux 2.4, right? It happens that in Linux 2.4, i2c_smbus_data has the
following definition:

union i2c_smbus_data {
    __u8 byte;
    __u16 word;
    __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
                      /* one more for read length in block process call */
};

So block has size 34 (I2C_SMBUS_BLOCK_MAX + 2), while i2c-CVS had 35
(I2C_SMBUS_BLOCK_MAX + 3) before my recent change. Now i2c-CVS has 34
again. The use of the last byte of block differs (supposedly block
process call in linux-2.4, actually unused, and PEC in i2c-CVS) but this
shouldn't cause any trouble.

I wouldn't actually trust alignment, as my understanding is that the
alignment of anything placed after an i2c_smbus_data depends on that
thing and not the definition of i2c_smbus_data. I see no reason why a u8
couldn't be placed right after an i2c_smbus_data, but I would expect an
u32 to be aligned on a 4-byte boundary. This might all depend on
architecture and compilation options though, I'm not sure.

I just took a look at all uses of i2c_smbus_data in Linux 2.4(.30), and
we seem to be lucky, i2c_smbus_data is always last on the stack or
followed by something requiring at least 4-byte alignment (int or
pointer). This must explain why the difference in the definition did
never cause compatibility problems.

--
Jean Delvare