* [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive
@ 2017-01-01 23:00 Laurent Vivier
2017-01-03 3:37 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2017-01-01 23:00 UTC (permalink / raw)
To: qemu-devel
Cc: Jason Wang, Hervé Poussineau, Aurelien Jarno, Laurent Vivier
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);
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)
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive
2017-01-01 23:00 [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive Laurent Vivier
@ 2017-01-03 3:37 ` Jason Wang
2017-01-03 7:26 ` Laurent Vivier
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2017-01-03 3:37 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: Hervé Poussineau, Aurelien Jarno
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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive
2017-01-03 3:37 ` Jason Wang
@ 2017-01-03 7:26 ` Laurent Vivier
2017-01-03 10:22 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2017-01-03 7:26 UTC (permalink / raw)
To: Jason Wang, qemu-devel; +Cc: Hervé Poussineau, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive
2017-01-03 7:26 ` Laurent Vivier
@ 2017-01-03 10:22 ` Jason Wang
2017-01-03 11:32 ` Laurent Vivier
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2017-01-03 10:22 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: Hervé Poussineau, Aurelien Jarno
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive
2017-01-03 10:22 ` Jason Wang
@ 2017-01-03 11:32 ` Laurent Vivier
0 siblings, 0 replies; 5+ messages in thread
From: Laurent Vivier @ 2017-01-03 11:32 UTC (permalink / raw)
To: Jason Wang, qemu-devel; +Cc: Hervé Poussineau, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-03 11:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-01 23:00 [Qemu-devel] [PATCH] dp8393x: fix dp8393x_receive Laurent Vivier
2017-01-03 3:37 ` Jason Wang
2017-01-03 7:26 ` Laurent Vivier
2017-01-03 10:22 ` Jason Wang
2017-01-03 11:32 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.