| Message ID | 20200210184139.342716-2-marex@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
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
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.
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.
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 ?
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.
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)
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(-)