[2/3] net: ks8851-ml: Fix 16-bit data access
diff mbox series

Message ID 20200210184139.342716-2-marex@denx.de
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • Untitled series #157750
Related show

Commit Message

Marek Vasut Feb. 10, 2020, 6:41 p.m. UTC
The packet data written to and read from Micrel KSZ8851-16MLLI must be
byte-swapped in 16-bit mode, add this byte-swapping.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851_mll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Lukas Wunner Feb. 10, 2020, 7:35 p.m. UTC | #1
On Mon, Feb 10, 2020 at 07:41:38PM +0100, Marek Vasut wrote:
> The packet data written to and read from Micrel KSZ8851-16MLLI must be
> byte-swapped in 16-bit mode, add this byte-swapping.
[...]
> -		*wptr++ = (u16)ioread16(ks->hw_addr);
> +		*wptr++ = swab16(ioread16(ks->hw_addr));

Um, doesn't this depend on the endianness of the CPU architecture?
I'd expect be16_to_cpu() or le16_to_cpu() here instead of swab16().

Thanks,

Lukas
Marek Vasut Feb. 10, 2020, 7:40 p.m. UTC | #2
On 2/10/20 8:35 PM, Lukas Wunner wrote:
> On Mon, Feb 10, 2020 at 07:41:38PM +0100, Marek Vasut wrote:
>> The packet data written to and read from Micrel KSZ8851-16MLLI must be
>> byte-swapped in 16-bit mode, add this byte-swapping.
> [...]
>> -		*wptr++ = (u16)ioread16(ks->hw_addr);
>> +		*wptr++ = swab16(ioread16(ks->hw_addr));
> 
> Um, doesn't this depend on the endianness of the CPU architecture?
> I'd expect be16_to_cpu() or le16_to_cpu() here instead of swab16().

I don't really know in this case, this is a bus IO accessor, so I would
expect the answer is no. But I can only test this on ARMv7a.
David Miller Feb. 15, 2020, 9:24 a.m. UTC | #3
From: Marek Vasut <marex@denx.de>
Date: Mon, 10 Feb 2020 19:41:38 +0100

> @@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
>  {
>  	len >>= 1;
>  	while (len--)
> -		*wptr++ = (u16)ioread16(ks->hw_addr);
> +		*wptr++ = swab16(ioread16(ks->hw_addr));

I agree with the other feedback, the endianness looks wrong here.

The ioread16() translates whatever is behind ks->hw_addr into cpu
endianness.

This is either always little endian (for example), which means that
unconditionally swapping this doesn't seem to make sense.
Marek Vasut Feb. 15, 2020, 9:47 a.m. UTC | #4
On 2/15/20 10:24 AM, David Miller wrote:
> From: Marek Vasut <marex@denx.de>
> Date: Mon, 10 Feb 2020 19:41:38 +0100
> 
>> @@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
>>  {
>>  	len >>= 1;
>>  	while (len--)
>> -		*wptr++ = (u16)ioread16(ks->hw_addr);
>> +		*wptr++ = swab16(ioread16(ks->hw_addr));
> 
> I agree with the other feedback, the endianness looks wrong here.
> 
> The ioread16() translates whatever is behind ks->hw_addr into cpu
> endianness.
> 
> This is either always little endian (for example), which means that
> unconditionally swapping this doesn't seem to make sense.

So what is the suggestion to fix this properly ?
David Miller Feb. 15, 2020, 9:56 a.m. UTC | #5
From: Marek Vasut <marex@denx.de>
Date: Sat, 15 Feb 2020 10:47:56 +0100

> On 2/15/20 10:24 AM, David Miller wrote:
>> From: Marek Vasut <marex@denx.de>
>> Date: Mon, 10 Feb 2020 19:41:38 +0100
>> 
>>> @@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
>>>  {
>>>  	len >>= 1;
>>>  	while (len--)
>>> -		*wptr++ = (u16)ioread16(ks->hw_addr);
>>> +		*wptr++ = swab16(ioread16(ks->hw_addr));
>> 
>> I agree with the other feedback, the endianness looks wrong here.
>> 
>> The ioread16() translates whatever is behind ks->hw_addr into cpu
>> endianness.
>> 
>> This is either always little endian (for example), which means that
>> unconditionally swapping this doesn't seem to make sense.
> 
> So what is the suggestion to fix this properly ?

once you know the endianness coming out of ioread16() you can then
use le16_to_cpu() or be16_to_cpu() as appropriate.

Patch
diff mbox series

diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
index e2fb20154511..e9ca198a67a4 100644
--- a/drivers/net/ethernet/micrel/ks8851_mll.c
+++ b/drivers/net/ethernet/micrel/ks8851_mll.c
@@ -197,7 +197,7 @@  static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
 {
 	len >>= 1;
 	while (len--)
-		*wptr++ = (u16)ioread16(ks->hw_addr);
+		*wptr++ = swab16(ioread16(ks->hw_addr));
 }
 
 /**
@@ -211,7 +211,7 @@  static inline void ks_outblk(struct ks_net *ks, u16 *wptr, u32 len)
 {
 	len >>= 1;
 	while (len--)
-		iowrite16(*wptr++, ks->hw_addr);
+		iowrite16(swab16(*wptr++), ks->hw_addr);
 }
 
 static void ks_disable_int(struct ks_net *ks)