| 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.
On Sat, Jan 04, 2025 at 11:32:59AM +0100, Wolfram Sang wrote: > > 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. Honestly, I'm not entirely happy with this patch. It seems to add unnecessary churn without addressing an actual need. Since we're operating in the same endianness domain, I don't think there is requirement for an endianness conversion here. On one hand, I want to give credit to Atharva for his effort; on the other hand, I think the real solution would be to just remove the comments altogether. I'm going to hold off for now and wait for additional feedback. If no further comments come in, I'll likely drop this patch. Andi
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(-)