Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

[Laurent Vivier <laurent@vivier.eu>]

Le 02/11/2019 à 20:41, Philippe Mathieu-Daudé a écrit :
> On 11/2/19 6:15 PM, Laurent Vivier wrote:
>> address_space_rw() access size must be multiplied by the width.
>>
>> This fixes DHCP for Q800 guest.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 85d3f3788e..b8c4473f99 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>> const uint8_t * buf,
>>       } else {
>>           dp8393x_put(s, width, 0, 0); /* in_use */
>>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>> * 6 * width,
>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>> sizeof(uint16_t), 1);
>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> 
> Which is following:
> 
>     if (s->regs[SONIC_LLFA] & 0x1) {
>         size = sizeof(uint16_t) * 1 * width;

but we have "size = sizeof(uint16_t) * width;" later.

This file needs cleanup...

> So OK (you describe 'width' but use 'size').
>

I meant the access size "sizeof(uint16_t)" must be adjusted by the width
of the bus (defined by (s->regs[SONIC_DCR] & SONIC_DCR_DW). [1]

And this is exactly what contains "size".

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Thanks,
Laurent

[1] DP83932C-20/25/33 MHz SONIC (tm) Systems-Oriented Network Interface
Controller

"1.3 DATA WIDTH AND BYTE ORDERING

The SONIC can be programmed to operate with either
32-bit or 16-bit wide memory. The data width is configured
during initialization by programming the DW bit in the Data
Configuration Register (DCR, Section 4.3.2). If the 16-bit
data path is selected, data is driven on pins D15– D0."