On Sat, Jul 3, 2021 at 6:32 PM BALATON Zoltan <bala...@eik.bme.hu> wrote:
> 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é <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 */
> > 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).
You are right indeed, this should be the opposite.
Thanks!
Phil.
