This is the mail archive of the ecos-discuss@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: bugs in AT91 Ethernet driver


> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: zaterdag 31 mei 2008 13:44
> To: Lambrecht Jürgen
> Cc: ecos-discuss@sources.redhat.com
> Subject: Re: [ECOS] bugs in AT91 Ethernet driver
> 
> Hi Joergen
(;-), my name is written with an 'u' with an umlaut (2 dots) on the u - and that umlaut-u gets screwed up often.. That's why I often type my name as Juergen because it is a German rule to put an 'e' after a letter that should have an umlaut but when you can/want not put one.)
> 
> 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.
> 
indeed, putting it at the beginning is more clear.

> @@ -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
indeed
Because 'bytes_in_list' was not reset, I thought this was an error. Only afterwards I found the 'bytes_in_list' error.

> means you have not got past the small ARP packets. The [bytes_in_list]
indeed, not got past the small ARP packets.

> 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.
> 
indeed
I knew this already, but I have made the error-i-thouht-it-was myself when writing my own fpga-memory-mapped Ethernet driver a while ago.
Then I thought sg_list was only 1 buffer and the 'i' was to iterate over the bytes.... Now I know better.

>    Andrew

Thanks Andrew.
This item is closed for the RX part.
For the TX part, see my other reply.

Kind regards,
Juergen

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]