* [PATCH] hw/char: sifive_uart: Free fifo on unrealize
@ 2025-03-03 2:31 Alistair Francis
2025-03-03 9:06 ` Daniel Henrique Barboza
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Alistair Francis @ 2025-03-03 2:31 UTC (permalink / raw)
To: palmer, liwei1518, dbarboza, zhiwei_liu, qemu-riscv, philmd,
chigot
Cc: qemu-devel, Alistair Francis
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
^ permalink raw reply related [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é
` (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
end of thread, other threads:[~2025-03-03 14:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).