On Mon, 15 Jun 2020 15:30:18 +0200 Pavel Machek pavel@denx.de wrote:
Hi!
From: Mathieu Othacehe m.othacehe@gmail.com
commit 18dfb5326370991c81a6d1ed6d1aeee055cb8c05 upstream.
The bytes returned by the i2c reading need to be swapped unconditionally. Otherwise, on be16 platforms, an incorrect value will be returned.
Taking the slow path via next merge window as its been around a while and we have a patch set dependent on this which would be held up.
Is there some explanation how this is correct Somewhere? I assume i2c hardware has fixed endianity (not depending on CPU), so unconditional swapping will cause problems either on le or on be machines...?
Hmm, this isn't introducing a bug, but looking again, it appears that the original code was fine as well..
smbus/I2C has a fixed ordering on the wire (when people actually obey the spec). So when an i2c_smbus_read_word_data call is made, the drivers / subsystem assume that ordering an provide the data in CPU endian order.
Unfortunately a reasonable set of devices provide data in the opposite order from that required by the i2c spec. Thus it needs to be unconditionally swapped to put it back into the correct order. i2c_smbus_read_word_data_swapped does that for us.
Now the reason it worked before was this was previously doing a block read rather than a word read. i2c_smbus_read_i2c_block_data which is just reading a bunch of bytes rather than doing a word read.
So this is a false positive as a fix. Sorry about that. Conversely it shouldn't break anything.
Looking back at the history, what happened was that up to v5 of this patchset factored out the reads, but whilst doing that converted them to i2c_smbus_read_word_data with a be16_to_cpu call which was buggy and picked up in review. Hence confusion occurred and I guess my eyes saw what they expected to see once the fix was pulled to the front of the series.
Thanks,
Jonathan
Best regards, Pavel