| Message ID | 20250303023120.157221-1-alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char: sifive_uart: Free fifo on unrealize | expand |
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;
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>
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!
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>
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;
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(-)