i.MX: Fix FEC/ENET receive funtions

Commit Message

Jean-Christophe Dubois Jan. 10, 2018, 8:38 p.m. UTC

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 <[e-mail]>
Tested-by: Fyleo <[e-mail]>
Signed-off-by: Jean-Christophe Dubois <[e-mail]>
---
 hw/net/imx_fec.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Comments

Peter Maydell Jan. 12, 2018, 5:08 p.m. UTC | #1

On 10 January 2018 at 20:38, Jean-Christophe Dubois <[e-mail]> 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 <[e-mail]>
> Tested-by: Fyleo <[e-mail]>
> Signed-off-by: Jean-Christophe Dubois <[e-mail]>

Could you have a look at current QEMU master, please? I think that
commit b2b012afdd9c has probably fixed this bug. (At any rate it
has changed that code so that your patch won't apply.)

thanks
-- PMM

Jean-Christophe Dubois Jan. 12, 2018, 5:24 p.m. UTC | #2

Le 2018-01-12 18:08, Peter Maydell a écrit :
> On 10 January 2018 at 20:38, Jean-Christophe Dubois 
> <[e-mail]> 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 <[e-mail]>
>> Tested-by: Fyleo <[e-mail]>
>> Signed-off-by: Jean-Christophe Dubois <[e-mail]>
> 
> Could you have a look at current QEMU master, please? I think that
> commit b2b012afdd9c has probably fixed this bug. (At any rate it
> has changed that code so that your patch won't apply.)

It seems the patch (imx_fec: Refactor imx_eth_enable_rx()) only renamed 
a variable (from tmp to rx_ring_full) without changing the logic. So I 
don't expect the bug to be fixed in mainline.

I'll rebase and resubmit my patch.

JC


> 
> thanks
> -- PMM

Patch

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 90e6ee35ba..04a5cf12f1 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -536,19 +536,16 @@  static void imx_eth_do_tx(IMXFECState *s)
 static void imx_eth_enable_rx(IMXFECState *s)
 {
     IMXFECBufDesc bd;
-    bool tmp;
 
     imx_fec_read_bd(&bd, s->rx_descriptor);
 
-    tmp = ((bd.flags & ENET_BD_E) != 0);
+    s->regs[ENET_RDAR] = (bd.flags & ENET_BD_E) ? ENET_RDAR_RDAR : 0;
 
-    if (!tmp) {
+    if (!s->regs[ENET_RDAR]) {
         FEC_PRINTF("RX buffer full\n");
-    } else if (!s->regs[ENET_RDAR]) {
+    } else {
         qemu_flush_queued_packets(qemu_get_queue(s->nic));
     }
-
-    s->regs[ENET_RDAR] = tmp ? ENET_RDAR_RDAR : 0;
 }
 
 static void imx_eth_reset(DeviceState *d)
@@ -806,7 +803,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);
             }
         } else {