* Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize
2025-03-03 2:31 [PATCH] hw/char: sifive_uart: Free fifo on unrealize Alistair Francis
@ 2025-03-03 9:06 ` Daniel Henrique Barboza
2025-03-03 10:04 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Daniel Henrique Barboza @ 2025-03-03 9:06 UTC (permalink / raw)
To: Alistair Francis, palmer, liwei1518, zhiwei_liu, qemu-riscv,
philmd, chigot
Cc: qemu-devel, Alistair Francis
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;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize
2025-03-03 2:31 [PATCH] hw/char: sifive_uart: Free fifo on unrealize Alistair Francis
2025-03-03 9:06 ` Daniel Henrique Barboza
@ 2025-03-03 10:04 ` Philippe Mathieu-Daudé
2025-03-03 14:23 ` Philippe Mathieu-Daudé
2025-03-03 14:46 ` Clément Chigot
3 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-03-03 10:04 UTC (permalink / raw)
To: Alistair Francis, palmer, liwei1518, dbarboza, zhiwei_liu,
qemu-riscv, chigot
Cc: qemu-devel, Alistair Francis
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize
2025-03-03 2:31 [PATCH] hw/char: sifive_uart: Free fifo on unrealize Alistair Francis
2025-03-03 9:06 ` Daniel Henrique Barboza
2025-03-03 10:04 ` Philippe Mathieu-Daudé
@ 2025-03-03 14:23 ` Philippe Mathieu-Daudé
2025-03-03 14:46 ` Clément Chigot
3 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-03-03 14:23 UTC (permalink / raw)
To: Alistair Francis, palmer, liwei1518, dbarboza, zhiwei_liu,
qemu-riscv, chigot
Cc: qemu-devel, Alistair Francis
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/char: sifive_uart: Free fifo on unrealize
2025-03-03 2:31 [PATCH] hw/char: sifive_uart: Free fifo on unrealize Alistair Francis
` (2 preceding siblings ...)
2025-03-03 14:23 ` Philippe Mathieu-Daudé
@ 2025-03-03 14:46 ` Clément Chigot
3 siblings, 0 replies; 5+ messages in thread
From: Clément Chigot @ 2025-03-03 14:46 UTC (permalink / raw)
To: Alistair Francis
Cc: palmer, liwei1518, dbarboza, zhiwei_liu, qemu-riscv, philmd,
qemu-devel, Alistair Francis
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>
^ permalink raw reply [flat|nested] 5+ messages in thread