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

[Jean-Christophe DUBOIS]

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  wrote:

On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell  wrote:

On 13 January 2018 at 11:34, Jean-Christophe Dubois  
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 
Tested-by: Fyleo  
Signed-off-by: Jean-Christophe Dubois 
---

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 

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 

Thanks; I've applied the patch to target-arm.next.

-- PMM

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

[Peter Maydell]

On 22 January 2018 at 14:54, Andrey Smirnov  wrote:
> On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell  
> wrote:
>> On 13 January 2018 at 11:34, Jean-Christophe Dubois  
>> 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 
>>> Tested-by: Fyleo  
>>> Signed-off-by: Jean-Christophe Dubois 
>>> ---
>>
>> 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 
>
> 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 

Thanks; I've applied the patch to target-arm.next.

-- PMM

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

[Andrey Smirnov]

On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell  wrote:
> On 13 January 2018 at 11:34, Jean-Christophe Dubois  
> 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 
>> Tested-by: Fyleo  
>> Signed-off-by: Jean-Christophe Dubois 
>> ---
>
> 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 

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 

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(, 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 
>
> thanks
> -- PMM

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

[Peter Maydell]

On 13 January 2018 at 11:34, Jean-Christophe Dubois  
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 
> Tested-by: Fyleo  
> Signed-off-by: Jean-Christophe Dubois 
> ---

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(, 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 

thanks
-- PMM

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

[Jean-Christophe Dubois]

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 
Tested-by: Fyleo  
Signed-off-by: Jean-Christophe Dubois 
---
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(, 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