Re: [PATCH 4/4] hw/net: xilinx_axienet: Fix DMA RX IRQ if ethernet disable RX

[Jim Shu]

Hi Peter,

Thanks for the suggestion.

axienet_eth_rx_notify() is also called by axidma_write() as a notify()
callback, so we need to check RCW1_RX in the function.
I think I could remove RCW1_RX checking in the enet_write() to avoid
double checking.
I will fix it in the v2 patchset.


Thanks,
JIm Shu.

On Mon, Jul 29, 2024 at 11:31 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
> >
> > When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA,
> > which may let AXI DMA to send RX full IRQs. It is aligned with real AXI
> > DMA/ethernet IP behavior in the FPGA.
> >
> > Also, now ethernet RX blocks the RX packets when it is disabled. It
> > should send and clear the remaining RX packets when enabling it.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++-----------------
> >  1 file changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> > index 05d41bd548..8428f10946 100644
> > --- a/hw/net/xilinx_axienet.c
> > +++ b/hw/net/xilinx_axienet.c
> > @@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, 
> > unsigned size)
> >      return r;
> >  }
> >
> > +static void axienet_eth_rx_notify(void *opaque)
> > +{
> > +    XilinxAXIEnet *s = XILINX_AXI_ENET(opaque);
> > +
> > +    /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ 
> > */
> > +    if (!axienet_rx_enabled(s)) {
> > +        return;
> > +    }
>
> This checks s->rcw[1] & RCW1_RX, and does nothing if it's not set...
>
> >  static void enet_write(void *opaque, hwaddr addr,
> >                         uint64_t value, unsigned size)
> >  {
> > @@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr,
> >              } else {
> >                  qemu_flush_queued_packets(qemu_get_queue(s->nic));
> >              }
> > +
> > +            /*
> > +             * When RX is enabled, check if any remaining data in rxmem
> > +             * and send them.
> > +             */
> > +            if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) {
> > +                axienet_eth_rx_notify(s);
> > +            }
>
> ...but at this callsite we open-code a check on RCW1_RX and
> skip the call if it's not set. We don't need to check twice.
>
> >              break;
> >
> >          case R_TC:
>
> thanks
> -- PMM