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

[Peter Maydell]

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

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 <address@hidden>

thanks
-- PMM