Re: [PATCH] Cadence: gem: fix wraparound in 64bit descriptors

[Peter Maydell]

On Thu, 16 Apr 2020 at 10:02, Ramon Fried <address@hidden> wrote:
>
> Wraparound of TX descriptor cyclic buffer only updated
> the low 32 bits of the descriptor.
> Fix that by checking if we're working with 64bit descriptors.
>
> Signed-off-by: Ramon Fried <address@hidden>
> ---
>  hw/net/cadence_gem.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 51ec5a072d..b8ae21cc0d 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1238,7 +1238,14 @@ static void gem_transmit(CadenceGEMState *s)
>              /* read next descriptor */
>              if (tx_desc_get_wrap(desc)) {
>                  tx_desc_set_last(desc);
> -                packet_desc_addr = s->regs[GEM_TXQBASE];
> +
> +            if (s->regs[GEM_DMACFG] & GEM_DMACFG_ADDR_64B) {
> +                packet_desc_addr = s->regs[GEM_TBQPH];
> +                packet_desc_addr <<= 32;
> +            } else {
> +                packet_desc_addr = 0;
> +            }
> +                packet_desc_addr |= s->regs[GEM_TXQBASE];

The indentation seems to be off here.

You could write this as:

    packet_desc_addr = s->regs[GEM_TXQBASE];
    if (s->regs[GEM_DMACFG] & GEM_DMACFG_ADDR_64B) {
        packet_desc_addr |= (uint64_t)s->regs[GEM_TBQPH] << 32;
    }

which ends up as the same thing but matches the code used
in tx_desc_get_buffer() to assemble an address that's
32 or 64 bits depending on the ADDR_64B flag.

>              } else {
>                  packet_desc_addr += 4 * gem_get_desc_len(s, false);
>              }
> --
> 2.26.0

thanks
-- PMM