bugfix in ethernet driver for the arm at91 variant: at91_eth.c

bugfix in ethernet driver for the arm at91 variant: at91_eth.c

[Bob Brusa]

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

Hi
I found a bug in the v3_0 eth-driver if_at91.c. It is in the handling of  
the buffer-to-list copy in routine at91_eth_recv. Actually, it only treats  
the first list and leaves all "higher" ones empty, thereby loosing data in  
packages that exceed the length of a single list.

The attached package updates adds a current version to the  
devs/eth/arm/at91 branch of ecos/packages.
   Robert



[-- Attachment #2: devs_current_100114.epk --]
[-- Type: application/octet-stream, Size: 9821 bytes --]

RE: bugfix in ethernet driver for the arm at91 variant: at91_eth.c

[Lambrecht Jürgen]

There several problems with that driver.
I improved it a lot a while ago, but need to clean it up to submit it back to ecos.
I putted it back on my todo list, for next month..

Regards,
Jürgen

> -----Original Message-----
> From: Bob Brusa [mailto:bob.brusa@gmail.com]
> Sent: donderdag 14 januari 2010 12:08
> To: ecos-patches@sources.redhat.com
> Subject: bugfix in ethernet driver for the arm at91 variant: at91_eth.c
>
> Hi
> I found a bug in the v3_0 eth-driver if_at91.c. It is in the handling
> of the buffer-to-list copy in routine at91_eth_recv. Actually, it only
> treats the first list and leaves all "higher" ones empty, thereby
> loosing data in packages that exceed the length of a single list.
>
> The attached package updates adds a current version to the
> devs/eth/arm/at91 branch of ecos/packages.
>    Robert
>

bugfix in ethernet driver for the arm at91 variant: at91_eth.c

[Bob Brusa]

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

Hi
This repeats a patch handed in already on 14-Jan-2010. But the epk-file  
then used does not correspond to the desired format. So I switched over to  
a textfile, obtained from a diff (see further down).

But back to the problem:
I found a bug in the v3_0 eth-driver if_at91.c. It is in the handling of
the buffer-to-list copy in routine at91_eth_recv. Actually, it only treats
the first list and leaves all "higher" ones empty, thereby loosing data in
packages that exceed the length of a single list.

The attached text file my.patch was generated using the command

cvs -q diff -u5 -w -p devs\eth\arm\at91 >> my.patch

Unfortunately as a newbie, I had added a lot of comments and all these
lines with new comments (which I think are useful) are now listed as  
changes.
     Robert

[-- Attachment #2: my.patch --]
[-- Type: application/octet-stream, Size: 11344 bytes --]

Index: devs/eth/arm/at91/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/ChangeLog,v
retrieving revision 1.4
diff -u -5 -w -p -r1.4 ChangeLog
--- devs/eth/arm/at91/current/ChangeLog	29 Jan 2009 17:47:59 -0000	1.4
+++ devs/eth/arm/at91/current/ChangeLog	15 Feb 2010 09:37:32 -0000
@@ -1,5 +1,11 @@
+2010-01-10  Robert Brusa <bob.brusa@gmail.com>
+
+	* src/if_at91.c: bug in buffer-to-list copy in routine
+	  at91_eth_recv fixed (It  handled the first list only
+	  correctly).
+	
 2007-04-08  Uwe Kindler  <uwe_kindler@web.de>
 
 	* cdl/at91_eth.cdl: Fixed typo. (Removed AT91RM9200 from
 	  CYGPKG_DEVS_ETH_ARM_AT91_DEBUG_LEVEL)
 	
Index: devs/eth/arm/at91/current/cdl/at91_eth.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/cdl/at91_eth.cdl,v
retrieving revision 1.3
diff -u -5 -w -p -r1.3 at91_eth.cdl
Index: devs/eth/arm/at91/current/src/if_at91.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/src/if_at91.c,v
retrieving revision 1.3
diff -u -5 -w -p -r1.3 if_at91.c
--- devs/eth/arm/at91/current/src/if_at91.c	29 Jan 2009 17:47:59 -0000	1.3
+++ devs/eth/arm/at91/current/src/if_at91.c	15 Feb 2010 09:37:32 -0000
@@ -37,13 +37,13 @@
 // -------------------------------------------                              
 // ####ECOSGPLCOPYRIGHTEND####                                              
 //==========================================================================
 //#####DESCRIPTIONBEGIN####
 //
-// Author(s):    Andrew Lunn, John Eigelaar
+// Author(s):    Andrew Lunn, John Eigelaar, Robert Brusa (rwb)
 // Contributors:  
-// Date:         2006-05-10
+// Date:         2006-05-10, 2010-01-10
 // Purpose:
 // Description:
 //
 //####DESCRIPTIONEND####
 //
@@ -919,76 +919,72 @@ at91_eth_deliver(struct eth_drv_sc *sc)
    {
       debug1_printf("AT91_ETH: Rx OVR\n");
    }
 
 }
-
+// rwb 100110 bugfix: list 0 ok, but higher lists where not handled correctly.
+// actually, the original code just ignored them.
 static void
 at91_eth_recv(struct eth_drv_sc *sc,
               struct eth_drv_sg *sg_list,
               int sg_len)
 {
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
    int i;
-   cyg_uint32 bytes_in_buffer;
-   cyg_uint32 bytes_in_list = 0;
-   cyg_uint32 bytes_needed_list = 0;
-   cyg_uint32 buffer_pos = 0;
-   cyg_uint8 * sg_buf;
-   cyg_uint32 total_bytes = 0;
+								    // number of bytes in ..
+   cyg_uint32 bytes_in_buffer;		// in hw-receive-buffer rbd
+   cyg_uint32 bytes_in_list;		// in current list
+   cyg_uint32 bytes_needed_list;	// required to make current list full
+   cyg_uint32 buffer_pos = 0;		// allready copied away from rec-buffer rbd
+   cyg_uint8 * sg_buf;				// pointer where to copy to
+   cyg_uint32 total_bytes = 0;		// total # of bytes copied from buffers.
 
    for(i = 0;i<sg_len;i++)
    {
+	 bytes_in_list = 0;	// current list is empty
+     sg_buf = (cyg_uint8 *)(sg_list[i].buf); // define copy-to-pointer
+
       while(bytes_in_list < sg_list[i].len)
-      {
+      {	// list i not full yet, compute how many bytes still have room in this
+    	// list i.
          bytes_needed_list = sg_list[i].len - bytes_in_list;
 
          if(priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_EOF)
-         {
-	      bytes_in_buffer = 
-		((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-		 - total_bytes) - buffer_pos;
+         { // eof-flag set
+        	 bytes_in_buffer = ((priv->rbd[priv->curr_rbd_idx].sr &
+        			 AT91_EMAC_RBD_SR_LEN_MASK) - total_bytes) - buffer_pos;
          }
          else
-         {
+         { // all the rest then
             bytes_in_buffer = AT91_EMAC_RX_BUFF_SIZE - buffer_pos;
          }
-
-         sg_buf = (cyg_uint8 *)(sg_list[i].buf);
-
          if(bytes_needed_list < bytes_in_buffer)
-         {
+         { // buffer has more bytes than there is room left in list
             if(sg_buf != NULL)
                memcpy(&sg_buf[bytes_in_list],
 		      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
-		      bytes_needed_list);
+		      bytes_needed_list);				// current list is full now
             bytes_in_list += bytes_needed_list;
             buffer_pos += bytes_needed_list;
             total_bytes += bytes_needed_list;
          }
          else
-         {
+         {	//rest of buffer fits into list. List may become full.
             if(sg_buf != NULL)
               memcpy(&sg_buf[bytes_in_list],
 		     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
 		     bytes_in_buffer);
             bytes_in_list += bytes_in_buffer;
             total_bytes += bytes_in_buffer;
-
-            /* Step our buffer on one */
-            priv->rbd[priv->curr_rbd_idx].addr &= 
-	      ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
-            priv->curr_rbd_idx++;
-            if(priv->curr_rbd_idx >= CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS)
-            {
-               priv->curr_rbd_idx = 0;
-            }
+         	// wee need a new buffer. Step our buffer on one
             buffer_pos = 0;
-         }
-      }
-   }
-}
+            priv->rbd[priv->curr_rbd_idx].addr &= ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
+            priv->curr_rbd_idx = (priv->curr_rbd_idx + 1) % CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS;
+         } // if !bytes...
+      } // while(bytes...
+   } // for (i
+} // end at91_eth_recv
 
 // routine called to handle ethernet controller in polled mode
 static void 
 at91_eth_poll(struct eth_drv_sc *sc)
 {
Index: devs/eth/arm/at91/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/ChangeLog,v
retrieving revision 1.4
diff -u -5 -w -p -r1.4 ChangeLog
--- devs/eth/arm/at91/current/ChangeLog	29 Jan 2009 17:47:59 -0000	1.4
+++ devs/eth/arm/at91/current/ChangeLog	15 Feb 2010 09:47:34 -0000
@@ -1,5 +1,11 @@
+2010-01-10  Robert Brusa <bob.brusa@gmail.com>
+
+	* src/if_at91.c: bug in buffer-to-list copy in routine
+	  at91_eth_recv fixed (It  handled the first list only
+	  correctly).
+	
 2007-04-08  Uwe Kindler  <uwe_kindler@web.de>
 
 	* cdl/at91_eth.cdl: Fixed typo. (Removed AT91RM9200 from
 	  CYGPKG_DEVS_ETH_ARM_AT91_DEBUG_LEVEL)
 	
Index: devs/eth/arm/at91/current/cdl/at91_eth.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/cdl/at91_eth.cdl,v
retrieving revision 1.3
diff -u -5 -w -p -r1.3 at91_eth.cdl
Index: devs/eth/arm/at91/current/src/if_at91.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/src/if_at91.c,v
retrieving revision 1.3
diff -u -5 -w -p -r1.3 if_at91.c
--- devs/eth/arm/at91/current/src/if_at91.c	29 Jan 2009 17:47:59 -0000	1.3
+++ devs/eth/arm/at91/current/src/if_at91.c	15 Feb 2010 09:47:35 -0000
@@ -37,13 +37,13 @@
 // -------------------------------------------                              
 // ####ECOSGPLCOPYRIGHTEND####                                              
 //==========================================================================
 //#####DESCRIPTIONBEGIN####
 //
-// Author(s):    Andrew Lunn, John Eigelaar
+// Author(s):    Andrew Lunn, John Eigelaar, Robert Brusa (rwb)
 // Contributors:  
-// Date:         2006-05-10
+// Date:         2006-05-10, 2010-01-10
 // Purpose:
 // Description:
 //
 //####DESCRIPTIONEND####
 //
@@ -919,76 +919,72 @@ at91_eth_deliver(struct eth_drv_sc *sc)
    {
       debug1_printf("AT91_ETH: Rx OVR\n");
    }
 
 }
-
+// rwb 100110 bugfix: list 0 ok, but higher lists where not handled correctly.
+// actually, the original code just ignored them.
 static void
 at91_eth_recv(struct eth_drv_sc *sc,
               struct eth_drv_sg *sg_list,
               int sg_len)
 {
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
    int i;
-   cyg_uint32 bytes_in_buffer;
-   cyg_uint32 bytes_in_list = 0;
-   cyg_uint32 bytes_needed_list = 0;
-   cyg_uint32 buffer_pos = 0;
-   cyg_uint8 * sg_buf;
-   cyg_uint32 total_bytes = 0;
+								    // number of bytes in ..
+   cyg_uint32 bytes_in_buffer;		// in hw-receive-buffer rbd
+   cyg_uint32 bytes_in_list;		// in current list
+   cyg_uint32 bytes_needed_list;	// required to make current list full
+   cyg_uint32 buffer_pos = 0;		// allready copied away from rec-buffer rbd
+   cyg_uint8 * sg_buf;				// pointer where to copy to
+   cyg_uint32 total_bytes = 0;		// total # of bytes copied from buffers.
 
    for(i = 0;i<sg_len;i++)
    {
+	 bytes_in_list = 0;	// current list is empty
+     sg_buf = (cyg_uint8 *)(sg_list[i].buf); // define copy-to-pointer
+
       while(bytes_in_list < sg_list[i].len)
-      {
+      {	// list i not full yet, compute how many bytes still have room in this
+    	// list i.
          bytes_needed_list = sg_list[i].len - bytes_in_list;
 
          if(priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_EOF)
-         {
-	      bytes_in_buffer = 
-		((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-		 - total_bytes) - buffer_pos;
+         { // eof-flag set
+        	 bytes_in_buffer = ((priv->rbd[priv->curr_rbd_idx].sr &
+        			 AT91_EMAC_RBD_SR_LEN_MASK) - total_bytes) - buffer_pos;
          }
          else
-         {
+         { // all the rest then
             bytes_in_buffer = AT91_EMAC_RX_BUFF_SIZE - buffer_pos;
          }
-
-         sg_buf = (cyg_uint8 *)(sg_list[i].buf);
-
          if(bytes_needed_list < bytes_in_buffer)
-         {
+         { // buffer has more bytes than there is room left in list
             if(sg_buf != NULL)
                memcpy(&sg_buf[bytes_in_list],
 		      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
-		      bytes_needed_list);
+		      bytes_needed_list);				// current list is full now
             bytes_in_list += bytes_needed_list;
             buffer_pos += bytes_needed_list;
             total_bytes += bytes_needed_list;
          }
          else
-         {
+         {	//rest of buffer fits into list. List may become full.
             if(sg_buf != NULL)
               memcpy(&sg_buf[bytes_in_list],
 		     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
 		     bytes_in_buffer);
             bytes_in_list += bytes_in_buffer;
             total_bytes += bytes_in_buffer;
-
-            /* Step our buffer on one */
-            priv->rbd[priv->curr_rbd_idx].addr &= 
-	      ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
-            priv->curr_rbd_idx++;
-            if(priv->curr_rbd_idx >= CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS)
-            {
-               priv->curr_rbd_idx = 0;
-            }
+         	// wee need a new buffer. Step our buffer on one
             buffer_pos = 0;
-         }
-      }
-   }
-}
+            priv->rbd[priv->curr_rbd_idx].addr &= ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
+            priv->curr_rbd_idx = (priv->curr_rbd_idx + 1) % CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS;
+         } // if !bytes...
+      } // while(bytes...
+   } // for (i
+} // end at91_eth_recv
 
 // routine called to handle ethernet controller in polled mode
 static void 
 at91_eth_poll(struct eth_drv_sc *sc)
 {

Re: bugfix in ethernet driver for the arm at91 variant: at91_eth.c

[Sergei Gavrikov]

Bob Brusa wrote:
> This repeats a patch handed in already on 14-Jan-2010. But the
> epk-file then used does not correspond to the desired format. So I
> switched over to a textfile, obtained from a diff (see further
> down).

Hi Robert,

Thanks for your contribution to eCos!

Robert, but, I see that Lambrecht Jürgen noticed us and you that he
plans to incorporate own changes for the at91 ethernet driver to eCos
soon: http://ecos.sourceware.org/ml/ecos-patches/2010-01/msg00003.html 

> I found a bug in the v3_0 eth-driver if_at91.c. It is in the
> handling of the buffer-to-list copy in routine at91_eth_recv.
> Actually, it only treats the first list and leaves all "higher" ones
> empty, thereby loosing data in packages that exceed the length of a
> single list.

Jürgen pointed on the several problems in the code. And it's possible
(I hope) that described the issue will be fixed in his patch. I just
afraid any duplicating efforts on the same code. The last record in
the driver's ChangeLog was done in 2007, so, any today's fixes or
improvements for the driver will be applicable for both v3_0 and cvs
driver code.

Now, two words about more important thing for any contribution to
eCos.  As I could see your patch is not trivial {1,10} lines patch.
So, you need to send own copy of a copyright assignment to eCos
maintainers. May be you've got the assignment (I'm sorry, I'm not
seeing you in the list), then let us to know, please.

References:
http://ecos.sourceware.org/contrib.html
http://ecos.sourceware.org/assign.html

> The attached text file my.patch was generated using the command
> 
> cvs -q diff -u5 -w -p devs\eth\arm\at91 >> my.patch
> 
> Unfortunately as a newbie, I had added a lot of comments and all
> these lines with new comments (which I think are useful) are now
> listed as changes.

From here
http://ecos.sourceware.org/docs-latest/ref/hal-porting-coding-conventions.html

you can know a bit on eCos coding style in common cases. Certainly, a
looking of eCos code itself will give you another clues for that how
eCos code must look. In common cases that's BSD indent style. Line
terminators must be NL ('\n') only. The originals have not CRs; So, if
your editor messes up the code, either tune the editor, please, or use
any kind of `dos2unix' program to remove the noise from the sources.

Thanks,

Sergei

bugfix in ethernet driver for the arm at91 variant: if_at91.c

[Bob Brusa]

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

Hi
This repeats a patch handed in already on 14-Jan-2010. But the epk-file
then used does not correspond to the desired format. So I switched over to
a textfile, obtained from a diff (see further down).

But back to the problem:
I found a bug in the v3_0 eth-driver if_at91.c. It is in the handling of
the buffer-to-list copy in routine at91_eth_recv. Actually, it only treats
the first list and leaves all "higher" ones empty, thereby loosing data in
packages that exceed the length of a single list.

The attached text file my.patch was generated using the command

cvs -q diff -u5 -w -p devs\eth\arm\at91 >> my.patch

Unfortunately as a newbie, I had added a lot of comments and all these
lines with new comments (which I think are useful) are now listed as
changes.
       Robert

Sorry, I found that the last my.patch sent included double entries and  
other "nonconformances". This latest version of the attached my.patch file  
is clean now:
No double entries
No tabs but spaces
No cr-lf, lf only

[-- Attachment #2: my.patch --]
[-- Type: application/octet-stream, Size: 5496 bytes --]

Index: devs/eth/arm/at91/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/ChangeLog,v
retrieving revision 1.4
diff -u -5 -w -p -r1.4 ChangeLog
--- devs/eth/arm/at91/current/ChangeLog	29 Jan 2009 17:47:59 -0000	1.4
+++ devs/eth/arm/at91/current/ChangeLog	15 Feb 2010 16:31:38 -0000
@@ -1,5 +1,11 @@
+2010-01-10  Robert Brusa <bob.brusa@gmail.com>
+
+        * src/if_at91.c: bug in buffer-to-list copy in routine
+          at91_eth_recv fixed (It  handled the first list only
+          correctly).
+        
 2007-04-08  Uwe Kindler  <uwe_kindler@web.de>
 
 	* cdl/at91_eth.cdl: Fixed typo. (Removed AT91RM9200 from
 	  CYGPKG_DEVS_ETH_ARM_AT91_DEBUG_LEVEL)
 	
Index: devs/eth/arm/at91/current/src/if_at91.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/arm/at91/current/src/if_at91.c,v
retrieving revision 1.3
diff -u -5 -w -p -r1.3 if_at91.c
--- devs/eth/arm/at91/current/src/if_at91.c	29 Jan 2009 17:47:59 -0000	1.3
+++ devs/eth/arm/at91/current/src/if_at91.c	15 Feb 2010 16:31:39 -0000
@@ -37,13 +37,13 @@
 // -------------------------------------------                              
 // ####ECOSGPLCOPYRIGHTEND####                                              
 //==========================================================================
 //#####DESCRIPTIONBEGIN####
 //
-// Author(s):    Andrew Lunn, John Eigelaar
+// Author(s):    Andrew Lunn, John Eigelaar, Robert Brusa (rwb)
 // Contributors:  
-// Date:         2006-05-10
+// Date:         2006-05-10, 2010-01-10
 // Purpose:
 // Description:
 //
 //####DESCRIPTIONEND####
 //
@@ -919,76 +919,72 @@ at91_eth_deliver(struct eth_drv_sc *sc)
    {
       debug1_printf("AT91_ETH: Rx OVR\n");
    }
 
 }
-
+// rwb 100110 bugfix: list 0 ok, but higher lists where not handled correctly.
+// actually, the original code just ignored them.
 static void
 at91_eth_recv(struct eth_drv_sc *sc,
               struct eth_drv_sg *sg_list,
               int sg_len)
 {
    at91_eth_priv_t *priv = (at91_eth_priv_t *)sc->driver_private;
    int i;
-   cyg_uint32 bytes_in_buffer;
-   cyg_uint32 bytes_in_list = 0;
-   cyg_uint32 bytes_needed_list = 0;
-   cyg_uint32 buffer_pos = 0;
-   cyg_uint8 * sg_buf;
-   cyg_uint32 total_bytes = 0;
+                    // number of bytes in ..
+   cyg_uint32 bytes_in_buffer;         // in hw-receive-buffer rbd
+   cyg_uint32 bytes_in_list;           // in current list
+   cyg_uint32 bytes_needed_list;       // required to make current list full
+   cyg_uint32 buffer_pos = 0;          // allready copied away from rec-buffer rbd
+   cyg_uint8 * sg_buf;                 // pointer where to copy to
+   cyg_uint32 total_bytes = 0;         // total # of bytes copied from buffers.
 
    for(i = 0;i<sg_len;i++)
    {
+   bytes_in_list = 0;                  // current list is empty
+     sg_buf = (cyg_uint8 *)(sg_list[i].buf); // define copy-to-pointer
+
       while(bytes_in_list < sg_list[i].len)
-      {
+      { // list i not full yet, compute how many bytes still have room in this
+      // list i.
          bytes_needed_list = sg_list[i].len - bytes_in_list;
 
          if(priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_EOF)
-         {
-	      bytes_in_buffer = 
-		((priv->rbd[priv->curr_rbd_idx].sr & AT91_EMAC_RBD_SR_LEN_MASK)
-		 - total_bytes) - buffer_pos;
+         { // eof-flag set
+           bytes_in_buffer = ((priv->rbd[priv->curr_rbd_idx].sr &
+               AT91_EMAC_RBD_SR_LEN_MASK) - total_bytes) - buffer_pos;
          }
          else
-         {
+         { // all the rest then
             bytes_in_buffer = AT91_EMAC_RX_BUFF_SIZE - buffer_pos;
          }
-
-         sg_buf = (cyg_uint8 *)(sg_list[i].buf);
-
          if(bytes_needed_list < bytes_in_buffer)
-         {
+         { // buffer has more bytes than there is room left in list
             if(sg_buf != NULL)
                memcpy(&sg_buf[bytes_in_list],
 		      &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
-		      bytes_needed_list);
+          bytes_needed_list);          // current list is full now
             bytes_in_list += bytes_needed_list;
             buffer_pos += bytes_needed_list;
             total_bytes += bytes_needed_list;
          }
          else
-         {
+         {  //rest of buffer fits into list. List may become full.
             if(sg_buf != NULL)
               memcpy(&sg_buf[bytes_in_list],
 		     &priv->rb[priv->curr_rbd_idx].rb[buffer_pos],
 		     bytes_in_buffer);
             bytes_in_list += bytes_in_buffer;
             total_bytes += bytes_in_buffer;
-
-            /* Step our buffer on one */
-            priv->rbd[priv->curr_rbd_idx].addr &= 
-	      ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
-            priv->curr_rbd_idx++;
-            if(priv->curr_rbd_idx >= CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS)
-            {
-               priv->curr_rbd_idx = 0;
-            }
+          // we need a new buffer. Step our buffer on one
             buffer_pos = 0;
-         }
-      }
-   }
-}
+            priv->rbd[priv->curr_rbd_idx].addr &= ~(AT91_EMAC_RBD_ADDR_OWNER_SW);
+            priv->curr_rbd_idx = (priv->curr_rbd_idx + 1) % CYGNUM_DEVS_ETH_ARM_AT91_RX_BUFS;
+         } // if !bytes...
+      } // while(bytes...
+   } // for (i
+} // end at91_eth_recv
 
 // routine called to handle ethernet controller in polled mode
 static void 
 at91_eth_poll(struct eth_drv_sc *sc)
 {

Re: bugfix in ethernet driver for the arm at91 variant: if_at91.c

[Sergei Gavrikov]

Bob Brusa wrote:
> Hi
> This repeats a patch handed in already on 14-Jan-2010. But the epk-file
> then used does not correspond to the desired format. So I switched over to
> a textfile, obtained from a diff (see further down).
> 
> But back to the problem:
> I found a bug in the v3_0 eth-driver if_at91.c. It is in the handling of
> the buffer-to-list copy in routine at91_eth_recv. Actually, it only treats
> the first list and leaves all "higher" ones empty, thereby loosing data in
> packages that exceed the length of a single list.

[snip]

> Sorry, I found that the last my.patch sent included double entries
> and other "nonconformances". This latest version of the attached
> my.patch file is clean now:
> No double entries
> No tabs but spaces
> No cr-lf, lf only

Thanks,

I'll take a look, but, as I said I would wait a while for the Jürgen's
fixes too to know about the possible interception/union boundaries for
the both workarounds.  And I very hope on that you help to provide a
testing on the target then, because, I have not the same hardware.

Sergei
--