Re: [ECOS] bugs in AT91 Ethernet driver

[Andrew Lunn <andrew at lunn dot ch>]

Hi Joergen

To make your patch easier to discuss, here it is again, but slightly
modified.

--- if_at91.c   23 Mar 2007 19:02:09 -0000      1.2
+++ if_at91.c   31 May 2008 11:31:39 -0000
@@ -937,6 +937,7 @@
 
    for(i = 0;i<sg_len;i++)
    {
+      bytes_in_list = 0;
       while(bytes_in_list < sg_list[i].len)
       {
          bytes_needed_list = sg_list[i].len - bytes_in_list;

You have this at the end. I put it at the beginning where it think it
more logically belongs. We have just started a new SG buffer, so we
currently have zero bytes in it. This i agree with.

@@ -945,7 +946,7 @@
          {
              bytes_in_buffer = 
                ((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-                - total_bytes) - buffer_pos;
+                - total_bytes);
          }
          else
          {

Your comment is correct. total_bytes already includes buffer_pos, so
including it again results in the driver discarding some bytes from
the end of the packet.

@@ -957,7 +958,7 @@
          if(bytes_needed_list < bytes_in_buffer)
          {
             if(sg_buf != NULL)
-               memcpy(&sg_buf[bytes_in_list],
+               memcpy(&sg_buf,
                      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
                      bytes_needed_list);
             bytes_in_list += bytes_needed_list;
@@ -967,7 +968,7 @@
          else
          {
             if(sg_buf != NULL)
-              memcpy(&sg_buf[bytes_in_list],
+              memcpy(&sg_buf,
                     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
                     bytes_in_buffer);
             bytes_in_list += bytes_in_buffer;

These two changes i don't understand. I think they are wrong. It could
be you have not found out this is wrong because of other problems
means you have not got past the small ARP packets. The [bytes_in_list]
is to handle when the bytes left in a receiver buffer are less than
the number of bytes left in the SG buffer. It needs to copy as many
bytes are available from one receive buffer and the move onto the next
receive buffer and copy as many bytes as needed into the sg buffer to
fill it up. Only then will it move onto the next sg buffer.

   Andrew


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss