* [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions
@ 2018-01-13 11:34 Jean-Christophe Dubois
2018-01-22 11:48 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Christophe Dubois @ 2018-01-13 11:34 UTC (permalink / raw)
To: jasowang
Cc: Jean-Christophe Dubois, qemu-devel, fyleo45, qemu-arm,
peter.maydell, peter.chubb
The actual imx_eth_enable_rx() function is buggy.
It updates s->regs[ENET_RDAR] after calling qemu_flush_queued_packets().
qemu_flush_queued_packets() is going to call imx_XXX_receive() which itself
is going to call imx_eth_enable_rx().
By updating s->regs[ENET_RDAR] after calling qemu_flush_queued_packets()
we end up updating the register with an outdated value which might
lead to disabling the receive function in the i.MX FEC/ENET device.
This patch change the place where the register update is done so that the
register value stays up to date and the receive function can keep
running.
Reported-by: Fyleo <fyleo45@gmail.com>
Tested-by: Fyleo <fyleo45@gmail.com>
Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
Change since v1:
1. Rebase the patch on the updated master branch
hw/net/imx_fec.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 4fb48f62ba..9506f9b69f 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -595,19 +595,16 @@ static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
static void imx_eth_enable_rx(IMXFECState *s, bool flush)
{
IMXFECBufDesc bd;
- bool rx_ring_full;
imx_fec_read_bd(&bd, s->rx_descriptor);
- rx_ring_full = !(bd.flags & ENET_BD_E);
+ s->regs[ENET_RDAR] = (bd.flags & ENET_BD_E) ? ENET_RDAR_RDAR : 0;
- if (rx_ring_full) {
+ if (!s->regs[ENET_RDAR]) {
FEC_PRINTF("RX buffer full\n");
} else if (flush) {
qemu_flush_queued_packets(qemu_get_queue(s->nic));
}
-
- s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
}
static void imx_eth_reset(DeviceState *d)
@@ -866,7 +863,6 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
case ENET_RDAR:
if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
if (!s->regs[index]) {
- s->regs[index] = ENET_RDAR_RDAR;
imx_eth_enable_rx(s, true);
}
} else {
--
2.14.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions
2018-01-13 11:34 [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions Jean-Christophe Dubois
@ 2018-01-22 11:48 ` Peter Maydell
2018-01-22 14:54 ` Andrey Smirnov
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2018-01-22 11:48 UTC (permalink / raw)
To: Jean-Christophe Dubois
Cc: Jason Wang, QEMU Developers, fyleo45, qemu-arm, Peter Chubb,
Andrey Smirnov
On 13 January 2018 at 11:34, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
> The actual imx_eth_enable_rx() function is buggy.
>
> It updates s->regs[ENET_RDAR] after calling qemu_flush_queued_packets().
>
> qemu_flush_queued_packets() is going to call imx_XXX_receive() which itself
> is going to call imx_eth_enable_rx().
>
> By updating s->regs[ENET_RDAR] after calling qemu_flush_queued_packets()
> we end up updating the register with an outdated value which might
> lead to disabling the receive function in the i.MX FEC/ENET device.
>
> This patch change the place where the register update is done so that the
> register value stays up to date and the receive function can keep
> running.
>
> Reported-by: Fyleo <fyleo45@gmail.com>
> Tested-by: Fyleo <fyleo45@gmail.com>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
Andrey, do you have an opinion on this patch, since you've been
looking at i.MX code recently?
> Change since v1:
> 1. Rebase the patch on the updated master branch
>
> hw/net/imx_fec.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 4fb48f62ba..9506f9b69f 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -595,19 +595,16 @@ static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
> static void imx_eth_enable_rx(IMXFECState *s, bool flush)
> {
> IMXFECBufDesc bd;
> - bool rx_ring_full;
>
> imx_fec_read_bd(&bd, s->rx_descriptor);
>
> - rx_ring_full = !(bd.flags & ENET_BD_E);
> + s->regs[ENET_RDAR] = (bd.flags & ENET_BD_E) ? ENET_RDAR_RDAR : 0;
>
> - if (rx_ring_full) {
> + if (!s->regs[ENET_RDAR]) {
> FEC_PRINTF("RX buffer full\n");
> } else if (flush) {
> qemu_flush_queued_packets(qemu_get_queue(s->nic));
> }
> -
> - s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
> }
>
> static void imx_eth_reset(DeviceState *d)
> @@ -866,7 +863,6 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
> case ENET_RDAR:
> if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
> if (!s->regs[index]) {
> - s->regs[index] = ENET_RDAR_RDAR;
> imx_eth_enable_rx(s, true);
> }
> } else {
> --
> 2.14.1
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions
2018-01-22 11:48 ` Peter Maydell
@ 2018-01-22 14:54 ` Andrey Smirnov
2018-01-22 14:59 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Smirnov @ 2018-01-22 14:54 UTC (permalink / raw)
To: Peter Maydell
Cc: Jean-Christophe Dubois, Jason Wang, QEMU Developers, fyleo45,
qemu-arm, Peter Chubb
On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 January 2018 at 11:34, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> The actual imx_eth_enable_rx() function is buggy.
>>
>> It updates s->regs[ENET_RDAR] after calling qemu_flush_queued_packets().
>>
>> qemu_flush_queued_packets() is going to call imx_XXX_receive() which itself
>> is going to call imx_eth_enable_rx().
>>
>> By updating s->regs[ENET_RDAR] after calling qemu_flush_queued_packets()
>> we end up updating the register with an outdated value which might
>> lead to disabling the receive function in the i.MX FEC/ENET device.
>>
>> This patch change the place where the register update is done so that the
>> register value stays up to date and the receive function can keep
>> running.
>>
>> Reported-by: Fyleo <fyleo45@gmail.com>
>> Tested-by: Fyleo <fyleo45@gmail.com>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> ---
>
> Andrey, do you have an opinion on this patch, since you've been
> looking at i.MX code recently?
>
The rationale makes sense to me and patch looks like a good cleanup in
general, so FWIW:
Reviewed-by: Andrey Smirnov <andrew.smirnov@gmail.com>
I also gave it a spin against my i.MX7 changes with doing basic things
like ping and scp of 1GB file, so I can give my:
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
for that.
Thanks,
Andrey Smirnov
>> Change since v1:
>> 1. Rebase the patch on the updated master branch
>>
>> hw/net/imx_fec.c | 8 ++------
>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 4fb48f62ba..9506f9b69f 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -595,19 +595,16 @@ static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
>> static void imx_eth_enable_rx(IMXFECState *s, bool flush)
>> {
>> IMXFECBufDesc bd;
>> - bool rx_ring_full;
>>
>> imx_fec_read_bd(&bd, s->rx_descriptor);
>>
>> - rx_ring_full = !(bd.flags & ENET_BD_E);
>> + s->regs[ENET_RDAR] = (bd.flags & ENET_BD_E) ? ENET_RDAR_RDAR : 0;
>>
>> - if (rx_ring_full) {
>> + if (!s->regs[ENET_RDAR]) {
>> FEC_PRINTF("RX buffer full\n");
>> } else if (flush) {
>> qemu_flush_queued_packets(qemu_get_queue(s->nic));
>> }
>> -
>> - s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
>> }
>>
>> static void imx_eth_reset(DeviceState *d)
>> @@ -866,7 +863,6 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>> case ENET_RDAR:
>> if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>> if (!s->regs[index]) {
>> - s->regs[index] = ENET_RDAR_RDAR;
>> imx_eth_enable_rx(s, true);
>> }
>> } else {
>> --
>> 2.14.1
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions
2018-01-22 14:54 ` Andrey Smirnov
@ 2018-01-22 14:59 ` Peter Maydell
2018-01-24 20:07 ` Jean-Christophe DUBOIS
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2018-01-22 14:59 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Jean-Christophe Dubois, Jason Wang, QEMU Developers, fyleo45,
qemu-arm, Peter Chubb
On 22 January 2018 at 14:54, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 13 January 2018 at 11:34, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>>> The actual imx_eth_enable_rx() function is buggy.
>>>
>>> It updates s->regs[ENET_RDAR] after calling qemu_flush_queued_packets().
>>>
>>> qemu_flush_queued_packets() is going to call imx_XXX_receive() which itself
>>> is going to call imx_eth_enable_rx().
>>>
>>> By updating s->regs[ENET_RDAR] after calling qemu_flush_queued_packets()
>>> we end up updating the register with an outdated value which might
>>> lead to disabling the receive function in the i.MX FEC/ENET device.
>>>
>>> This patch change the place where the register update is done so that the
>>> register value stays up to date and the receive function can keep
>>> running.
>>>
>>> Reported-by: Fyleo <fyleo45@gmail.com>
>>> Tested-by: Fyleo <fyleo45@gmail.com>
>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>> ---
>>
>> Andrey, do you have an opinion on this patch, since you've been
>> looking at i.MX code recently?
>>
>
> The rationale makes sense to me and patch looks like a good cleanup in
> general, so FWIW:
>
> Reviewed-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> I also gave it a spin against my i.MX7 changes with doing basic things
> like ping and scp of 1GB file, so I can give my:
>
> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Thanks; I've applied the patch to target-arm.next.
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions
2018-01-22 14:59 ` Peter Maydell
@ 2018-01-24 20:07 ` Jean-Christophe DUBOIS
0 siblings, 0 replies; 5+ messages in thread
From: Jean-Christophe DUBOIS @ 2018-01-24 20:07 UTC (permalink / raw)
To: Peter Maydell, Andrey Smirnov
Cc: fyleo45, Jason Wang, QEMU Developers, qemu-arm, Peter Chubb
My guess is that with this patch, the "flush" feature that was added by
Andrey in "imx_fec: Change queue flushing heuristics" (commit b2b012a)
is not really necessary anymore.
But it does not hurt (it might induce a little bit more processing).
JC
Le 22/01/2018 à 15:59, Peter Maydell a écrit :
> On 22 January 2018 at 14:54, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 13 January 2018 at 11:34, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>>>> The actual imx_eth_enable_rx() function is buggy.
>>>>
>>>> It updates s->regs[ENET_RDAR] after calling qemu_flush_queued_packets().
>>>>
>>>> qemu_flush_queued_packets() is going to call imx_XXX_receive() which itself
>>>> is going to call imx_eth_enable_rx().
>>>>
>>>> By updating s->regs[ENET_RDAR] after calling qemu_flush_queued_packets()
>>>> we end up updating the register with an outdated value which might
>>>> lead to disabling the receive function in the i.MX FEC/ENET device.
>>>>
>>>> This patch change the place where the register update is done so that the
>>>> register value stays up to date and the receive function can keep
>>>> running.
>>>>
>>>> Reported-by: Fyleo <fyleo45@gmail.com>
>>>> Tested-by: Fyleo <fyleo45@gmail.com>
>>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>>> ---
>>> Andrey, do you have an opinion on this patch, since you've been
>>> looking at i.MX code recently?
>>>
>> The rationale makes sense to me and patch looks like a good cleanup in
>> general, so FWIW:
>>
>> Reviewed-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>
>> I also gave it a spin against my i.MX7 changes with doing basic things
>> like ping and scp of 1GB file, so I can give my:
>>
>> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Thanks; I've applied the patch to target-arm.next.
>
> -- PMM
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-24 20:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-13 11:34 [Qemu-devel] [PATCH v2] i.MX: Fix FEC/ENET receive funtions Jean-Christophe Dubois
2018-01-22 11:48 ` Peter Maydell
2018-01-22 14:54 ` Andrey Smirnov
2018-01-22 14:59 ` Peter Maydell
2018-01-24 20:07 ` Jean-Christophe DUBOIS
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.