Re: [PATCH v3 6/8] dp8393x: Store CRC using device configured endianess

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> Little-Endian CRC is dubious. The datasheet does not
> specify it being little-endian. Use big-endian access
> when the device is configured in such endianess.
> (This is a theoretical bug fix.)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/net/dp8393x.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 68516241a1f..ac93412f70b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -827,7 +827,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const 
> uint8_t * buf,
>       s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
>   
>       /* Calculate the ethernet checksum */
> -    checksum = cpu_to_le32(crc32(0, buf, pkt_size));
> +    checksum = crc32(0, buf, pkt_size);
>   
>       /* Put packet into RBA */
>       trace_dp8393x_receive_packet(dp8393x_crba(s));
> @@ -837,8 +837,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const 
> uint8_t * buf,
>       address += pkt_size;
>   
>       /* Put frame checksum into RBA */
> -    address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> -                        &checksum, sizeof(checksum));
> +    if (s->big_endian) {
> +        address_space_stl_be(&s->as, address, checksum,
> +                             MEMTXATTRS_UNSPECIFIED, NULL);
> +    } else {
> +        address_space_stl_le(&s->as, address, checksum,
> +                             MEMTXATTRS_UNSPECIFIED, NULL);
> +    }
>       address += sizeof(checksum);
>   
>       /* Pad short packets to keep pointers aligned */

This is obviously new to the series: I can test this on big endian m68k but are you 
sure that this won't break big endian MIPS? Or do we not care for now since we don't 
have a working test image?


ATB,

Mark.