drivers/i2c/busses/i2c-amd756.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
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(-)
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;
--
2.39.5Hi 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: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
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+
> 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?
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
> 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
> 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. Yes. Atharva can still send a patch doing this.
© 2016 - 2026 Red Hat, Inc.