[Qemu-devel] [PATCH] i.MX: Fix FEC/ENET receive funtions

Jean-Christophe Dubois posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180110203823.22292-1-jcd@tribudubois.net
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/net/imx_fec.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH] i.MX: Fix FEC/ENET receive funtions
Posted by Jean-Christophe Dubois 6 years, 2 months ago
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>
---
 hw/net/imx_fec.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

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 {
-- 
2.14.1


Re: [Qemu-devel] [PATCH] i.MX: Fix FEC/ENET receive funtions
Posted by Peter Maydell 6 years, 2 months ago
On 10 January 2018 at 20:38, 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>

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

Re: [Qemu-devel] [PATCH] i.MX: Fix FEC/ENET receive funtions
Posted by Jean-Christophe Dubois 6 years, 2 months ago
Le 2018-01-12 18:08, Peter Maydell a écrit :
> On 10 January 2018 at 20:38, 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>
> 
> 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