Re: [PATCH-for-8.2 v2 2/2] hw/net/can/xlnx-zynqmp: Avoid underflow while popping RX FIFO

[Vikram Garhwal]

On Wed, Nov 22, 2023 at 08:45:56PM +0100, Francisco Iglesias wrote:
> On Sun, Nov 19, 2023 at 11:51:02PM +0100, Philippe Mathieu-Daudé wrote:
> > Per 
> > https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Message-Format
> > 
> >   Message Format
> > 
> >   The same message format is used for RXFIFO, TXFIFO, and TXHPB.
> >   Each message includes four words (16 bytes). Software must read
> >   and write all four words regardless of the actual number of data
> >   bytes and valid fields in the message.
> > 
> > There is no mention in this reference manual about what the
> > hardware does when not all four words are read. To fix the
> > reported underflow behavior, I choose to fill the 4 frame data
> > registers when the first register (ID) is accessed, which is how
> > I expect hardware would do.
> > 
> > Reported-by: Qiang Liu <cyruscyliu@gmail.com>
> > Fixes: 98e5d7a2b7 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1427
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
Reviewed-by: Vikram Garhwal <vikram.garhwal@amd.com>

> 
> 
> > ---
> >  hw/net/can/xlnx-zynqmp-can.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
> > index 58938b574e..c63fb4a83c 100644
> > --- a/hw/net/can/xlnx-zynqmp-can.c
> > +++ b/hw/net/can/xlnx-zynqmp-can.c
> > @@ -777,14 +777,18 @@ static void update_rx_fifo(XlnxZynqMPCANState *s, 
> > const qemu_can_frame *frame)
> >      }
> >  }
> >  
> > -static uint64_t can_rxfifo_pre_read(RegisterInfo *reg, uint64_t val)
> > +static uint64_t can_rxfifo_post_read_id(RegisterInfo *reg, uint64_t val)
> >  {
> >      XlnxZynqMPCANState *s = XLNX_ZYNQMP_CAN(reg->opaque);
> > +    unsigned used = fifo32_num_used(&s->rx_fifo);
> >  
> > -    if (!fifo32_is_empty(&s->rx_fifo)) {
> > -        val = fifo32_pop(&s->rx_fifo);
> > -    } else {
> > +    if (used < CAN_FRAME_SIZE) {
> >          ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXUFLW, 1);
> > +    } else {
> > +        val = s->regs[R_RXFIFO_ID] = fifo32_pop(&s->rx_fifo);
> > +        s->regs[R_RXFIFO_DLC] = fifo32_pop(&s->rx_fifo);
> > +        s->regs[R_RXFIFO_DATA1] = fifo32_pop(&s->rx_fifo);
> > +        s->regs[R_RXFIFO_DATA2] = fifo32_pop(&s->rx_fifo);
> >      }
> >  
> >      can_update_irq(s);
> > @@ -945,14 +949,11 @@ static const RegisterAccessInfo can_regs_info[] = {
> >          .post_write = can_tx_post_write,
> >      },{ .name = "RXFIFO_ID",  .addr = A_RXFIFO_ID,
> >          .ro = 0xffffffff,
> > -        .post_read = can_rxfifo_pre_read,
> > +        .post_read = can_rxfifo_post_read_id,
> >      },{ .name = "RXFIFO_DLC",  .addr = A_RXFIFO_DLC,
> >          .rsvd = 0xfff0000,
> > -        .post_read = can_rxfifo_pre_read,
> >      },{ .name = "RXFIFO_DATA1",  .addr = A_RXFIFO_DATA1,
> > -        .post_read = can_rxfifo_pre_read,
> >      },{ .name = "RXFIFO_DATA2",  .addr = A_RXFIFO_DATA2,
> > -        .post_read = can_rxfifo_pre_read,
> >      },{ .name = "AFR",  .addr = A_AFR,
> >          .rsvd = 0xfffffff0,
> >          .post_write = can_filter_enable_post_write,
> > -- 
> > 2.41.0
> >