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.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4...@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 */
I don't understand this comment. Why would you check the CRC when it's
meant to be discarded here? (This is the CRCI enabled case.)
> 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) {
> + rx_len += sizeof(checksum);
> + }
> if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> padded_len = ((rx_len - 1) | 3) + 1;
> } else {
This is in dp8393x_receive(), but CRCI does not apply to the recieve side
of the chip.
> @@ -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) {
Same mistake here.
> + /* 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) {
>
Anyway, I think you are right about the FCS endianness error (which you
address in the next patch).
