[RFC PATCH 0/3] dp8393x: Reviewing CRC code

[RFC PATCH 0/3] dp8393x: Reviewing CRC code

[Philippe Mathieu-Daudé]

Hi Mark, few more patches while reviewing.

Again, not tested (yet)... Simply compiled.

Please tell me what you think of them.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  dp8393x: Store CRC using address_space_stl_le()
  dp8393x: Do not amend CRC if it is inhibited (CRCI bit set)
  dp8393x: Store CRC using device configured endianess

 hw/net/dp8393x.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

-- 
2.31.1

[RFC PATCH 1/3] dp8393x: Store CRC using address_space_stl_le()

[Philippe Mathieu-Daudé]

The address_space API can handle endianess conversion.
Replace cpu_to_le32() + address_space_write() by a
single address_space_stl_le() call.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index db9cfd786f5..99e179a5e86 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -802,7 +802,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));
@@ -812,8 +812,8 @@ 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));
+    address_space_stl_le(&s->as, address, checksum, MEMTXATTRS_UNSPECIFIED,
+                         NULL);
     address += sizeof(checksum);
 
     /* Pad short packets to keep pointers aligned */
-- 
2.31.1

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

[Philippe Mathieu-Daudé]

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 */
             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) {
-- 
2.31.1

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

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) {
>

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

[Philippe Mathieu-Daudé]

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.

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

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]

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).

[RFC PATCH 3/3] dp8393x: Store CRC using device configured endianess

[Philippe Mathieu-Daudé]

Little-Endian CRC is dubious, and the datasheet does not specify
it being little-endian. Proceed similarly with the other memory
accesses, use the device endianess.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/dp8393x.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index dee8236400c..3a07f5c8ac9 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -817,8 +817,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         checksum = crc32(0, buf, pkt_size);
 
         /* Put frame checksum into RBA */
-        address_space_stl_le(&s->as, address, checksum, MEMTXATTRS_UNSPECIFIED,
-                             NULL);
+        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);
     }
 
-- 
2.31.1

Re: [RFC PATCH 0/3] dp8393x: Reviewing CRC code

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]


On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote:

> Hi Mark, few more patches while reviewing.
> 
> 
> 
> Again, not tested (yet)... Simply compiled.
> 
> 
> 
> Please tell me what you think of them.
> 
> 

I think these 3 patches can be reduced to this theoretical bug fix:

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index db9cfd786f..e278daebc5 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -802,7 +802,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));
@@ -812,8 +812,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 */

> 
> Regards,
> 
> 
> 
> Phil.
> 
> 
> 
> Philippe Mathieu-Daudé (3):
> 
>   dp8393x: Store CRC using address_space_stl_le()
> 
>   dp8393x: Do not amend CRC if it is inhibited (CRCI bit set)
> 
>   dp8393x: Store CRC using device configured endianess
> 
> 
> 
>  hw/net/dp8393x.c | 26 ++++++++++++++++++--------
> 
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> 
> 
> -- 
> 
> 2.31.1
> 
> 
> 
>