diff mbox series

[PULL,08/10] hw/net/can/xlnx-zynqmp-can: fix assertion failures in transfer_fifo()

Message ID 20230214061140.36696-9-jasowang@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/10] net: Move the code to collect available NIC models to a separate function | expand

Commit Message

Jason Wang Feb. 14, 2023, 6:11 a.m. UTC
From: Qiang Liu <cyruscyliu@gmail.com>

Check fifos before poping data from and pushing data into it.

Fixes: 98e5d7a2b726 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1425
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1427
Reported-by: Qiang Liu <cyruscyliu@gmail.com>
Signed-off-by: Qiang Liu <cyruscyliu@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/can/xlnx-zynqmp-can.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Feb. 15, 2023, 2:56 p.m. UTC | #1
Adding CAN bus maintainers.

On 14/2/23 07:11, Jason Wang wrote:
> From: Qiang Liu <cyruscyliu@gmail.com>
> 
> Check fifos before poping data from and pushing data into it.
> 
> Fixes: 98e5d7a2b726 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1425
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1427
> Reported-by: Qiang Liu <cyruscyliu@gmail.com>
> Signed-off-by: Qiang Liu <cyruscyliu@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/net/can/xlnx-zynqmp-can.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
> index e93e6c5..55d3221 100644
> --- a/hw/net/can/xlnx-zynqmp-can.c
> +++ b/hw/net/can/xlnx-zynqmp-can.c
> @@ -451,6 +451,12 @@ static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo)
>       }
>   
>       while (!fifo32_is_empty(fifo)) {
> +        if (fifo32_num_used(fifo) < (4 * CAN_FRAME_SIZE)) {
> +            g_autofree char *path = object_get_canonical_path(OBJECT(s));
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: data left in the fifo is not"
> +                          " enough for transfer.\n", path);
> +            break;

This change looks dubious... Shouldn't this rejected earlier?
Shouldn't we assert(fifo32_num_used(fifo)) >= CAN_FRAME_SIZE here?
Is this really how this works on the hardware?

>           for (i = 0; i < CAN_FRAME_SIZE; i++) {
>               data[i] = fifo32_pop(fifo);
>           }
diff mbox series

Patch

diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
index e93e6c5..55d3221 100644
--- a/hw/net/can/xlnx-zynqmp-can.c
+++ b/hw/net/can/xlnx-zynqmp-can.c
@@ -451,6 +451,12 @@  static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo)
     }
 
     while (!fifo32_is_empty(fifo)) {
+        if (fifo32_num_used(fifo) < (4 * CAN_FRAME_SIZE)) {
+            g_autofree char *path = object_get_canonical_path(OBJECT(s));
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: data left in the fifo is not"
+                          " enough for transfer.\n", path);
+            break;
+        }
         for (i = 0; i < CAN_FRAME_SIZE; i++) {
             data[i] = fifo32_pop(fifo);
         }
@@ -463,7 +469,8 @@  static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo)
              * acknowledged. The XlnxZynqMPCAN core receives any message
              * that it transmits.
              */
-            if (fifo32_is_full(&s->rx_fifo)) {
+            if (fifo32_is_full(&s->rx_fifo) ||
+                    (fifo32_num_free(&s->rx_fifo) < (4 * CAN_FRAME_SIZE))) {
                 ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXOFLW, 1);
             } else {
                 for (i = 0; i < CAN_FRAME_SIZE; i++) {