hw/char: sifive_uart: Free fifo on unrealize

[PATCH] hw/char: sifive_uart: Free fifo on unrealize

Posted by Alistair Francis 1 day, 19 hours ago

We previously allocate the fifo on reset and never free it, which means
we are leaking memory.

Instead let's allocate on realize and free on unrealize.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/char/sifive_uart.c | 44 +++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 4bc5767284..b45e6c098c 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -251,6 +251,23 @@ static int sifive_uart_be_change(void *opaque)
     return 0;
 }
 
+static void sifive_uart_reset_enter(Object *obj, ResetType type)
+{
+    SiFiveUARTState *s = SIFIVE_UART(obj);
+
+    s->txfifo = 0;
+    s->ie = 0;
+    s->ip = 0;
+    s->txctrl = 0;
+    s->rxctrl = 0;
+    s->div = 0;
+
+    s->rx_fifo_len = 0;
+
+    memset(s->rx_fifo, 0, SIFIVE_UART_RX_FIFO_SIZE);
+    fifo8_reset(&s->tx_fifo);
+}
+
 static const Property sifive_uart_properties[] = {
     DEFINE_PROP_CHR("chardev", SiFiveUARTState, chr),
 };
@@ -270,30 +287,24 @@ static void sifive_uart_realize(DeviceState *dev, Error **errp)
 {
     SiFiveUARTState *s = SIFIVE_UART(dev);
 
+    fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
+
     s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                           fifo_trigger_update, s);
 
-    qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
-                             sifive_uart_event, sifive_uart_be_change, s,
-                             NULL, true);
+    if (qemu_chr_fe_backend_connected(&s->chr)) {
+        qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
+                                 sifive_uart_event, sifive_uart_be_change, s,
+                                 NULL, true);
+    }
 
 }
 
-static void sifive_uart_reset_enter(Object *obj, ResetType type)
+static void sifive_uart_unrealize(DeviceState *dev)
 {
-    SiFiveUARTState *s = SIFIVE_UART(obj);
-
-    s->txfifo = 0;
-    s->ie = 0;
-    s->ip = 0;
-    s->txctrl = 0;
-    s->rxctrl = 0;
-    s->div = 0;
-
-    s->rx_fifo_len = 0;
+    SiFiveUARTState *s = SIFIVE_UART(dev);
 
-    memset(s->rx_fifo, 0, SIFIVE_UART_RX_FIFO_SIZE);
-    fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
+    fifo8_destroy(&s->tx_fifo);
 }
 
 static void sifive_uart_reset_hold(Object *obj, ResetType type)
@@ -329,6 +340,7 @@ static void sifive_uart_class_init(ObjectClass *oc, void *data)
     ResettableClass *rc = RESETTABLE_CLASS(oc);
 
     dc->realize = sifive_uart_realize;
+    dc->unrealize = sifive_uart_unrealize;
     dc->vmsd = &vmstate_sifive_uart;
     rc->phases.enter = sifive_uart_reset_enter;
     rc->phases.hold  = sifive_uart_reset_hold;
-- 
2.48.1

Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize

Posted by Clément Chigot 1 day, 7 hours ago

On Mon, Mar 3, 2025 at 3:31 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> We previously allocate the fifo on reset and never free it, which means
> we are leaking memory.
>
> Instead let's allocate on realize and free on unrealize.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/char/sifive_uart.c | 44 +++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
> index 4bc5767284..b45e6c098c 100644
> --- a/hw/char/sifive_uart.c
> +++ b/hw/char/sifive_uart.c
> @@ -251,6 +251,23 @@ static int sifive_uart_be_change(void *opaque)
>      return 0;
>  }
>
> +static void sifive_uart_reset_enter(Object *obj, ResetType type)
> +{
> +    SiFiveUARTState *s = SIFIVE_UART(obj);
> +
> +    s->txfifo = 0;
> +    s->ie = 0;
> +    s->ip = 0;
> +    s->txctrl = 0;
> +    s->rxctrl = 0;
> +    s->div = 0;
> +
> +    s->rx_fifo_len = 0;
> +
> +    memset(s->rx_fifo, 0, SIFIVE_UART_RX_FIFO_SIZE);
> +    fifo8_reset(&s->tx_fifo);
> +}
> +
>  static const Property sifive_uart_properties[] = {
>      DEFINE_PROP_CHR("chardev", SiFiveUARTState, chr),
>  };
> @@ -270,30 +287,24 @@ static void sifive_uart_realize(DeviceState *dev, Error **errp)
>  {
>      SiFiveUARTState *s = SIFIVE_UART(dev);
>
> +    fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
> +
>      s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                            fifo_trigger_update, s);
>
> -    qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
> -                             sifive_uart_event, sifive_uart_be_change, s,
> -                             NULL, true);
> +    if (qemu_chr_fe_backend_connected(&s->chr)) {
> +        qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
> +                                 sifive_uart_event, sifive_uart_be_change, s,
> +                                 NULL, true);
> +    }
>
>  }
>
> -static void sifive_uart_reset_enter(Object *obj, ResetType type)
> +static void sifive_uart_unrealize(DeviceState *dev)
>  {
> -    SiFiveUARTState *s = SIFIVE_UART(obj);
> -
> -    s->txfifo = 0;
> -    s->ie = 0;
> -    s->ip = 0;
> -    s->txctrl = 0;
> -    s->rxctrl = 0;
> -    s->div = 0;
> -
> -    s->rx_fifo_len = 0;
> +    SiFiveUARTState *s = SIFIVE_UART(dev);
>
> -    memset(s->rx_fifo, 0, SIFIVE_UART_RX_FIFO_SIZE);
> -    fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
> +    fifo8_destroy(&s->tx_fifo);
>  }
>
>  static void sifive_uart_reset_hold(Object *obj, ResetType type)
> @@ -329,6 +340,7 @@ static void sifive_uart_class_init(ObjectClass *oc, void *data)
>      ResettableClass *rc = RESETTABLE_CLASS(oc);
>
>      dc->realize = sifive_uart_realize;
> +    dc->unrealize = sifive_uart_unrealize;
>      dc->vmsd = &vmstate_sifive_uart;
>      rc->phases.enter = sifive_uart_reset_enter;
>      rc->phases.hold  = sifive_uart_reset_hold;
> --
> 2.48.1
>

Thanks for the patch.

Tested-by: Clément Chigot <chigot@adacore.com>

Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize

Posted by Philippe Mathieu-Daudé 1 day, 8 hours ago

On 3/3/25 03:31, Alistair Francis wrote:
> We previously allocate the fifo on reset and never free it, which means
> we are leaking memory.
> 
> Instead let's allocate on realize and free on unrealize.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   hw/char/sifive_uart.c | 44 +++++++++++++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 16 deletions(-)

Patch queued, thanks!

Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize

Posted by Philippe Mathieu-Daudé 1 day, 12 hours ago

On 3/3/25 03:31, Alistair Francis wrote:
> We previously allocate the fifo on reset and never free it, which means
> we are leaking memory.
> 
> Instead let's allocate on realize and free on unrealize.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   hw/char/sifive_uart.c | 44 +++++++++++++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize

Posted by Daniel Henrique Barboza 1 day, 13 hours ago


On 3/2/25 11:31 PM, Alistair Francis wrote:
> We previously allocate the fifo on reset and never free it, which means
> we are leaking memory.
> 
> Instead let's allocate on realize and free on unrealize.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   hw/char/sifive_uart.c | 44 +++++++++++++++++++++++++++----------------
>   1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
> index 4bc5767284..b45e6c098c 100644
> --- a/hw/char/sifive_uart.c
> +++ b/hw/char/sifive_uart.c
> @@ -251,6 +251,23 @@ static int sifive_uart_be_change(void *opaque)
>       return 0;
>   }
>   
> +static void sifive_uart_reset_enter(Object *obj, ResetType type)
> +{
> +    SiFiveUARTState *s = SIFIVE_UART(obj);
> +
> +    s->txfifo = 0;
> +    s->ie = 0;
> +    s->ip = 0;
> +    s->txctrl = 0;
> +    s->rxctrl = 0;
> +    s->div = 0;
> +
> +    s->rx_fifo_len = 0;
> +
> +    memset(s->rx_fifo, 0, SIFIVE_UART_RX_FIFO_SIZE);
> +    fifo8_reset(&s->tx_fifo);
> +}
> +
>   static const Property sifive_uart_properties[] = {
>       DEFINE_PROP_CHR("chardev", SiFiveUARTState, chr),
>   };
> @@ -270,30 +287,24 @@ static void sifive_uart_realize(DeviceState *dev, Error **errp)
>   {
>       SiFiveUARTState *s = SIFIVE_UART(dev);
>   
> +    fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
> +
>       s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                             fifo_trigger_update, s);
>   
> -    qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
> -                             sifive_uart_event, sifive_uart_be_change, s,
> -                             NULL, true);
> +    if (qemu_chr_fe_backend_connected(&s->chr)) {
> +        qemu_chr_fe_set_handlers(&s->chr, sifive_uart_can_rx, sifive_uart_rx,
> +                                 sifive_uart_event, sifive_uart_be_change, s,
> +                                 NULL, true);
> +    }
>   
>   }
>   
> -static void sifive_uart_reset_enter(Object *obj, ResetType type)
> +static void sifive_uart_unrealize(DeviceState *dev)
>   {
> -    SiFiveUARTState *s = SIFIVE_UART(obj);
> -
> -    s->txfifo = 0;
> -    s->ie = 0;
> -    s->ip = 0;
> -    s->txctrl = 0;
> -    s->rxctrl = 0;
> -    s->div = 0;
> -
> -    s->rx_fifo_len = 0;
> +    SiFiveUARTState *s = SIFIVE_UART(dev);
>   
> -    memset(s->rx_fifo, 0, SIFIVE_UART_RX_FIFO_SIZE);
> -    fifo8_create(&s->tx_fifo, SIFIVE_UART_TX_FIFO_SIZE);
> +    fifo8_destroy(&s->tx_fifo);
>   }
>   
>   static void sifive_uart_reset_hold(Object *obj, ResetType type)
> @@ -329,6 +340,7 @@ static void sifive_uart_class_init(ObjectClass *oc, void *data)
>       ResettableClass *rc = RESETTABLE_CLASS(oc);
>   
>       dc->realize = sifive_uart_realize;
> +    dc->unrealize = sifive_uart_unrealize;
>       dc->vmsd = &vmstate_sifive_uart;
>       rc->phases.enter = sifive_uart_reset_enter;
>       rc->phases.hold  = sifive_uart_reset_hold;