| Message ID | 20250101103422.30523-1-evepolonium@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: amd756: Fix endianness handling for word data | expand |
On Wed, Jan 01, 2025 at 04:04:22PM +0530, Atharva Tiwari wrote: > Ensure correct handling of "endianness" > for word-sized data in amd756_access > > - Convert word data into little-endian using cpu_to_le16 > - Convert word data from little-endian > to cpu native format using le16_to_cpu > > This fixes poteential issues on big-endian systems and > ensure proper byte ordering for SMBus word transacitions > > and you would be thinking why did i resend the patch > it is because kernel test robot > noticed a few warning so i fixed them first of all, thanks for your patch. I was aware that this issue had been reported by the LKP, but I initially decided to keep it as it was and queue it in i2c/i2c-host since I didn't consider it a serious issue. > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202412311145.AKMzVNw4-lkp@intel.com/ Now, you're treating it as a fix, which is perfectly fine. For me, it's a 50-50 case. I'll reward your second version by moving this patch to the fixes branch. However, for the next time, please: - Be transparent about your intentions: you knew I had already merged it, and you should have discussed this with me before sending a new version. - Number your patches as [PATCH v2] using 'git format-patch -v 2...'. - Avoid leaving blank spaces in the tag section: for example, there is a blank line between 'Closes:' and 'Signed-off-by:'. - Add a changelog to highlight what has changed. - Include the 'Fixes:' tag[1] (wow, this dates back to the origins of git!). - Cc the stable mailing list[2] to ensure proper visibility. In any case, don't worry—I’ll take care of this, and there's no need for you to resend it. Thanks, Andi [1] Fixes: 1da177e4c3f4 ('Linux-2.6.12-rc2') [2] Cc: <stable@vger.kernel.org> # v2.6.12+
Hi again, > @@ -211,7 +212,7 @@ static s32 amd756_access(struct i2c_adapter * adap, u16 addr, > SMB_HOST_ADDRESS); > outb_p(command, SMB_HOST_COMMAND); > if (read_write == I2C_SMBUS_WRITE) > - outw_p(data->word, SMB_HOST_DATA); /* TODO: endian???? */ > + outw_p(cpu_to_le16((u16)data->word), SMB_HOST_DATA); > size = AMD756_WORD_DATA; > break; > case I2C_SMBUS_BLOCK_DATA: > @@ -256,7 +257,7 @@ static s32 amd756_access(struct i2c_adapter * adap, u16 addr, > data->byte = inw_p(SMB_HOST_DATA); > break; > case AMD756_WORD_DATA: > - data->word = inw_p(SMB_HOST_DATA); /* TODO: endian???? */ > + data->word = (u16)le16_to_cpu(inw_p(SMB_HOST_DATA)); sorry, please do send a new version, the cast should not be needed here. If you have any questions, feel free to ask, after having read Documentation/process/coding-style.rst. Thanks, Andi > break; > case AMD756_BLOCK_DATA: > data->block[0] = inw_p(SMB_HOST_DATA) & 0x3f; > -- > 2.39.5 >
On Sat, Jan 04, 2025 at 12:28:47AM +0100, Wolfram Sang wrote: > > Now, you're treating it as a fix, which is perfectly fine. For > > me, it's a 50-50 case. > > For me, this is a clear no-fix case as long as nobody really reported > problems (which I am not aware of). Also, looking at the Kconfig text, > it looks unlikely to me that this controller has been used with big > endian systems. Or? Yeah! That was my first thought, but because this was reported by LKP (which I respect more than other code analyzers) as a potential issue, I was on the 50-50. I still agree that the patch is correct because there's a hanging comment. So, either remove the comment or go with Athrva's approach. Urgh, I'm just leaving things as they are :-) Andi
On Sat, Jan 04, 2025 at 12:32:39AM +0100, Andi Shyti wrote: > Hi again, > > > @@ -211,7 +212,7 @@ static s32 amd756_access(struct i2c_adapter * adap, u16 addr, > > SMB_HOST_ADDRESS); > > outb_p(command, SMB_HOST_COMMAND); > > if (read_write == I2C_SMBUS_WRITE) > > - outw_p(data->word, SMB_HOST_DATA); /* TODO: endian???? */ > > + outw_p(cpu_to_le16((u16)data->word), SMB_HOST_DATA); > > size = AMD756_WORD_DATA; > > break; > > case I2C_SMBUS_BLOCK_DATA: > > @@ -256,7 +257,7 @@ static s32 amd756_access(struct i2c_adapter * adap, u16 addr, > > data->byte = inw_p(SMB_HOST_DATA); > > break; > > case AMD756_WORD_DATA: > > - data->word = inw_p(SMB_HOST_DATA); /* TODO: endian???? */ > > + data->word = (u16)le16_to_cpu(inw_p(SMB_HOST_DATA)); > > sorry, please do send a new version, the cast should not be > needed here. > > If you have any questions, feel free to ask, after having read > Documentation/process/coding-style.rst. Sorry, I managed to make myself misunderstood in my previous emails. Please don't send anything, I've already pushed the version with the fix reported by LKP. Andi
> Yeah! That was my first thought, but because this was reported > by LKP (which I respect more than other code analyzers) as a > potential issue, I was on the 50-50. A potential issue on outdated hardware (or better: non-existing versions of outdated harware) which has not showed up since the git era is not really a for-current bugfix in my book. That being said, it does not harm in for-current.
diff --git a/drivers/i2c/busses/i2c-amd756.c b/drivers/i2c/busses/i2c-amd756.c index fa0d5a2c3732..e551d63e96b1 100644 --- a/drivers/i2c/busses/i2c-amd756.c +++ b/drivers/i2c/busses/i2c-amd756.c @@ -31,6 +31,7 @@ #include <linux/i2c.h> #include <linux/acpi.h> #include <linux/io.h> +#include <linux/byteorder/generic.h> /* AMD756 SMBus address offsets */ #define SMB_ADDR_OFFSET 0xE0 @@ -211,7 +212,7 @@ static s32 amd756_access(struct i2c_adapter * adap, u16 addr, SMB_HOST_ADDRESS); outb_p(command, SMB_HOST_COMMAND); if (read_write == I2C_SMBUS_WRITE) - outw_p(data->word, SMB_HOST_DATA); /* TODO: endian???? */ + outw_p(cpu_to_le16((u16)data->word), SMB_HOST_DATA); size = AMD756_WORD_DATA; break; case I2C_SMBUS_BLOCK_DATA: @@ -256,7 +257,7 @@ static s32 amd756_access(struct i2c_adapter * adap, u16 addr, data->byte = inw_p(SMB_HOST_DATA); break; case AMD756_WORD_DATA: - data->word = inw_p(SMB_HOST_DATA); /* TODO: endian???? */ + data->word = (u16)le16_to_cpu(inw_p(SMB_HOST_DATA)); break; case AMD756_BLOCK_DATA: data->block[0] = inw_p(SMB_HOST_DATA) & 0x3f;
Ensure correct handling of "endianness" for word-sized data in amd756_access - Convert word data into little-endian using cpu_to_le16 - Convert word data from little-endian to cpu native format using le16_to_cpu This fixes poteential issues on big-endian systems and ensure proper byte ordering for SMBus word transacitions and you would be thinking why did i resend the patch it is because kernel test robot noticed a few warning so i fixed them Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202412311145.AKMzVNw4-lkp@intel.com/ Signed-off-by: Atharva Tiwari <evepolonium@gmail.com> --- drivers/i2c/busses/i2c-amd756.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)