Re: [RFC PATCH 2/3] dp8393x: Do not amend CRC if it is inhibited (CRCI bit set)

[BALATON Zoltan]

On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote:
> When the CRCI (CRC INHIBIT) bit is set, the 4-byte FCS field
> is not transmitted.

You say when CRCI is 1 then no checksum...

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 99e179a5e86..dee8236400c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -472,6 +472,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>              */
>         } else {
>             /* Remove existing FCS */
> +            /* TODO check crc */
>             tx_len -= 4;
>             if (tx_len < 0) {
>                 trace_dp8393x_transmit_txlen_error(tx_len);
> @@ -758,7 +759,10 @@ static ssize_t dp8393x_receive(NetClientState *nc, const 
> uint8_t * buf,
>         return pkt_size;
>     }
>
> -    rx_len = pkt_size + sizeof(checksum);
> +    rx_len = pkt_size;
> +    if (s->regs[SONIC_TCR] & SONIC_TCR_CRCI) {

... but you seem to add checksum if bit is set.

> +        rx_len += sizeof(checksum);
> +    }
>     if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
>         padded_len = ((rx_len - 1) | 3) + 1;
>     } else {
> @@ -801,9 +805,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const 
> uint8_t * buf,
>     s->regs[SONIC_TRBA1] = s->regs[SONIC_CRBA1];
>     s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
>
> -    /* Calculate the ethernet checksum */
> -    checksum = crc32(0, buf, pkt_size);
> -
>     /* Put packet into RBA */
>     trace_dp8393x_receive_packet(dp8393x_crba(s));
>     address = dp8393x_crba(s);
> @@ -811,10 +812,15 @@ static ssize_t dp8393x_receive(NetClientState *nc, const 
> uint8_t * buf,
>                         buf, pkt_size);
>     address += pkt_size;
>
> -    /* Put frame checksum into RBA */
> -    address_space_stl_le(&s->as, address, checksum, MEMTXATTRS_UNSPECIFIED,
> -                         NULL);
> -    address += sizeof(checksum);
> +    if (s->regs[SONIC_TCR] & SONIC_TCR_CRCI) {

Here too. Either these should be inverted or the commit message is wrong 
if the bit is active 0 (or I'm not getting this alltogether which is also 
possible as I've just had a quick look without really understanding it).

Regards,
BALATON Zoltan

> +        /* Calculate the ethernet checksum */
> +        checksum = crc32(0, buf, pkt_size);
> +
> +        /* Put frame checksum into RBA */
> +        address_space_stl_le(&s->as, address, checksum, MEMTXATTRS_UNSPECIFIED,
> +                             NULL);
> +        address += sizeof(checksum);
> +    }
>
>     /* Pad short packets to keep pointers aligned */
>     if (rx_len < padded_len) {