| Message ID | 20210703150219.364582-3-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dp8393x: Reviewing CRC code | expand |
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) { >
On Sat, Jul 3, 2021 at 6:32 PM BALATON Zoltan <balaton@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é <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). You are right indeed, this should be the opposite. Thanks! Phil.
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é <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 */ 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).
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) { + 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) { + /* 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) {
When the CRCI (CRC INHIBIT) bit is set, the 4-byte FCS field is not transmitted. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)