[PATCH] i2c: amd756: Fix endianness handling for word data

[PATCH] i2c: amd756: Fix endianness handling for word data

[Atharva Tiwari]

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.5

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Andi Shyti]

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+  

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]


> 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Andi Shyti]

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

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Andi Shyti]

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

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Andi Shyti]

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
> 

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Andi Shyti]

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

[PATCH] i2c: amd756: Fix endianness handling for word data

[Atharva Tiwari]

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

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..f6fe4c32af04 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(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 = le16_to_cpu(inw_p(SMB_HOST_DATA));
 		break;
 	case AMD756_BLOCK_DATA:
 		data->block[0] = inw_p(SMB_HOST_DATA) & 0x3f;
-- 
2.39.5

Re: [PATCH] i2c: amd756: Fix endianness handling for word data

[Andi Shyti]

Hi Atharva,

On Fri, Dec 27, 2024 at 06:52:49PM +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
> 
> Signed-off-by: Atharva Tiwari <evepolonium@gmail.com>

merged to i2c/i2c-host.

Thanks,
Andi