Re: [Qemu-devel] [PATCH 2/2] net: Fix lan9118 buffer length handling

[Roy Franz]

On Fri, Dec 27, 2013 at 5:25 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Sat, Dec 21, 2013 at 4:26 AM, Roy Franz <address@hidden> wrote:
>> The 9118 ethernet controller supports transmission of multi-buffer packets
>> with arbitrary byte alignment of the start and end bytes.  All writes to
>> the packet fifo are 32 bits, so the controller discards bytes at the 
>> beginning
>> and end of each buffer based on the 'Data start offset' and 'Buffer size'
>> of the TX command 'A' format.
>>
>> This patch uses the provided buffer length to limit the bytes transmitted.
>> Previously all the bytes of the last 32-bit word written to the TX fifo
>> were added to the internal transmit buffer structure resulting in more bytes
>> being transmitted than were submitted to the hardware in the command.  This
>> resulted in extra bytes being inserted into the middle of multi-buffer
>> packets when the non-final buffers had non-32bit aligned ending addresses.
>>
>> Signed-off-by: Roy Franz <address@hidden>
>> ---
>>  hw/net/lan9118.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
>> index c5d6f14..712bb41 100644
>> --- a/hw/net/lan9118.c
>> +++ b/hw/net/lan9118.c
>> @@ -773,11 +773,10 @@ static void tx_fifo_push(lan9118_state *s, uint32_t 
>> val)
>>                 in FIFO words.  Empirical results show it to be 
>> little-endian.
>>                 */
>>              /* TODO: FIFO overflow checking.  */
>> -            while (n--) {
>> +            while (n-- && s->txp->buffer_size--) {
>
> can you just get n right in the first place (line 766):
>
> n = MIN(4, s->txp_buffer_size);
>
> rather than doing min calculation on two parallel iteration variables?
>
> Regards,
> Peter
>
>>                  s->txp->data[s->txp->len] = val & 0xff;
>>                  s->txp->len++;
>>                  val >>= 8;
>> -                s->txp->buffer_size--;
>>              }
>>              s->txp->fifo_used++;
>>          }
>> --
>> 1.7.10.4
>>
>>

Hi Peter,

   I'll take a look at this (and your other comments on the extract32)
when I am back at work next week.

Thanks,
Roy