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

[Ramon Fried]

On Fri, Apr 17, 2020 at 4:37 PM Peter Maydell <address@hidden> wrote:
>
> 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.
Right, fixing.
>
> 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;
>     }
>
Actually, I used the same method used somewhere else in the file:
static hwaddr gem_get_desc_addr(CadenceGEMState *s, bool tx, int q) {
hwaddr desc_addr = 0;

if (s->regs[GEM_DMACFG] & GEM_DMACFG_ADDR_64B) {
         desc_addr = s->regs[tx ? GEM_TBQPH : GEM_RBQPH];
     }
     desc_addr <<= 32;
     desc_addr |= tx ? s->tx_desc_addr[q] : s->rx_desc_addr[q];
}
> 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

Thanks,
Ramon.