diff mbox

dp8393x: fix dp8393x_receive

Message ID 1483311635-27905-1-git-send-email-laurent@vivier.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Vivier Jan. 1, 2017, 11 p.m. UTC 
address_space_rw() access size must be multiplied by width.
dp8393x_receive() must return the number of bytes read, not the length
of the last memory access.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jason Wang Jan. 3, 2017, 3:37 a.m. UTC | #1 
On 2017年01月02日 07:00, Laurent Vivier wrote:
> address_space_rw() access size must be multiplied by width.
> dp8393x_receive() must return the number of bytes read, not the length
> of the last memory access.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 17f0338..3506bca 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -766,10 +766,11 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
> +        size = sizeof(uint16_t) * width;
>           data[0 * width] = 0; /* in_use */
>           address_space_rw(&s->as,
>               ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 6 * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, sizeof(uint16_t), 1);
> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);

I think patch makes different only when width is 2. But if we just want 
to tag in_use flag, sizeof(uint16_t) should be sufficient?

Thanks

>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> @@ -783,7 +784,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       /* Done */
>       dp8393x_update_irq(s);
>   
> -    return size;
> +    return rx_len;
>   }
>   
>   static void dp8393x_reset(DeviceState *dev)
Laurent Vivier Jan. 3, 2017, 7:26 a.m. UTC | #2 
Le 03/01/2017 à 04:37, Jason Wang a écrit :
> 
> 
> On 2017年01月02日 07:00, Laurent Vivier wrote:
>> address_space_rw() access size must be multiplied by width.
>> dp8393x_receive() must return the number of bytes read, not the length
>> of the last memory access.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 17f0338..3506bca 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -766,10 +766,11 @@ static ssize_t dp8393x_receive(NetClientState
>> *nc, const uint8_t * buf,
>>           /* EOL detected */
>>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>>       } else {
>> +        size = sizeof(uint16_t) * width;
>>           data[0 * width] = 0; /* in_use */
>>           address_space_rw(&s->as,
>>               ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) +
>> sizeof(uint16_t) * 6 * width,
>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data,
>> sizeof(uint16_t), 1);
>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
> 
> I think patch makes different only when width is 2. But if we just want
> to tag in_use flag, sizeof(uint16_t) should be sufficient?

In fact, I'm using this device to implement Quadra 800 macsonic
emulation, and in this case all registers are byteswapped according the
size of width (32bit in my case, see kernel sources,
drivers/net/ethernet/natsemi/sonic.h and macsonic.c), so debugging the
kernel I've seen used part of the register is never cleared.

Thanks,
Laurent
Jason Wang Jan. 3, 2017, 10:22 a.m. UTC | #3 
On 2017年01月03日 15:26, Laurent Vivier wrote:
> Le 03/01/2017 à 04:37, Jason Wang a écrit :
>>
>> On 2017年01月02日 07:00, Laurent Vivier wrote:
>>> address_space_rw() access size must be multiplied by width.
>>> dp8393x_receive() must return the number of bytes read, not the length
>>> of the last memory access.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>    hw/net/dp8393x.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 17f0338..3506bca 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -766,10 +766,11 @@ static ssize_t dp8393x_receive(NetClientState
>>> *nc, const uint8_t * buf,
>>>            /* EOL detected */
>>>            s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>>>        } else {
>>> +        size = sizeof(uint16_t) * width;
>>>            data[0 * width] = 0; /* in_use */
>>>            address_space_rw(&s->as,
>>>                ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) +
>>> sizeof(uint16_t) * 6 * width,
>>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data,
>>> sizeof(uint16_t), 1);
>>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
>> I think patch makes different only when width is 2. But if we just want
>> to tag in_use flag, sizeof(uint16_t) should be sufficient?
> In fact, I'm using this device to implement Quadra 800 macsonic
> emulation, and in this case all registers are byteswapped according the
> size of width (32bit in my case, see kernel sources,
> drivers/net/ethernet/natsemi/sonic.h and macsonic.c), so debugging the
> kernel I've seen used part of the register is never cleared.

Looking at git history this seems a revert of 
409b52bfe199d8106dadf7c5ff3d88d2228e89b5 ("net/dp8393x: correctly reset 
in_use field") which claims to fix an issue in NetBSD 5.1. According to 
the spec (is this correct 
http://www.ti.com/lit/ds/symlink/dp83936avul-33.pdf?) upper 16 bits were 
not used in 32bit mode, so I'm not sure this is correct.

Hope Hervé or Aurelien can review this patch.

Thanks

> Thanks,
> Laurent
>
Laurent Vivier Jan. 3, 2017, 11:32 a.m. UTC | #4 
Le 03/01/2017 à 11:22, Jason Wang a écrit :
> 
> 
> On 2017年01月03日 15:26, Laurent Vivier wrote:
>> Le 03/01/2017 à 04:37, Jason Wang a écrit :
>>>
>>> On 2017年01月02日 07:00, Laurent Vivier wrote:
>>>> address_space_rw() access size must be multiplied by width.
>>>> dp8393x_receive() must return the number of bytes read, not the length
>>>> of the last memory access.
>>>>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>>    hw/net/dp8393x.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>>> index 17f0338..3506bca 100644
>>>> --- a/hw/net/dp8393x.c
>>>> +++ b/hw/net/dp8393x.c
>>>> @@ -766,10 +766,11 @@ static ssize_t dp8393x_receive(NetClientState
>>>> *nc, const uint8_t * buf,
>>>>            /* EOL detected */
>>>>            s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>>>>        } else {
>>>> +        size = sizeof(uint16_t) * width;
>>>>            data[0 * width] = 0; /* in_use */
>>>>            address_space_rw(&s->as,
>>>>                ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) +
>>>> sizeof(uint16_t) * 6 * width,
>>>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data,
>>>> sizeof(uint16_t), 1);
>>>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
>>> I think patch makes different only when width is 2. But if we just want
>>> to tag in_use flag, sizeof(uint16_t) should be sufficient?
>> In fact, I'm using this device to implement Quadra 800 macsonic
>> emulation, and in this case all registers are byteswapped according the
>> size of width (32bit in my case, see kernel sources,
>> drivers/net/ethernet/natsemi/sonic.h and macsonic.c), so debugging the
>> kernel I've seen used part of the register is never cleared.
> 
> Looking at git history this seems a revert of
> 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 ("net/dp8393x: correctly reset
> in_use field") which claims to fix an issue in NetBSD 5.1. According to
> the spec (is this correct
> http://www.ti.com/lit/ds/symlink/dp83936avul-33.pdf?) upper 16 bits were
> not used in 32bit mode, so I'm not sure this is correct.

You should be right as the linux kernel only writes a 16bit value even
if width is 32 bits. I'm going to rework this.

Thanks,
Laurent
diff mbox

Patch

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 17f0338..3506bca 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -766,10 +766,11 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
+        size = sizeof(uint16_t) * width;
         data[0 * width] = 0; /* in_use */
         address_space_rw(&s->as,
             ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 6 * width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, sizeof(uint16_t), 1);
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
@@ -783,7 +784,7 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     /* Done */
     dp8393x_update_irq(s);
 
-    return size;
+    return rx_len;
 }
 
 static void dp8393x_reset(DeviceState *dev)