Fixes: 35593573b25 ("hw/dma: Implement a Xilinx CSU DMA model")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/dma/xlnx_csu_dma.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index 797b4fed8f5..0c1c19cab5a 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -660,6 +660,13 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp)
s->r_size_last_word = 0;
}
+static void xlnx_csu_dma_unrealize(DeviceState *dev)
+{
+ XlnxCSUDMA *s = XLNX_CSU_DMA(dev);
+
+ ptimer_free(s->src_timer);
+}
+
static const VMStateDescription vmstate_xlnx_csu_dma = {
.name = TYPE_XLNX_CSU_DMA,
.version_id = 0,
@@ -700,6 +707,7 @@ static void xlnx_csu_dma_class_init(ObjectClass *klass, void *data)
dc->reset = xlnx_csu_dma_reset;
dc->realize = xlnx_csu_dma_realize;
+ dc->unrealize = xlnx_csu_dma_unrealize;
dc->vmsd = &vmstate_xlnx_csu_dma;
device_class_set_props(dc, xlnx_csu_dma_properties);
--
2.31.1
On Thu, 19 Aug 2021 at 15:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Fixes: 35593573b25 ("hw/dma: Implement a Xilinx CSU DMA model") > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/dma/xlnx_csu_dma.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c > index 797b4fed8f5..0c1c19cab5a 100644 > --- a/hw/dma/xlnx_csu_dma.c > +++ b/hw/dma/xlnx_csu_dma.c > @@ -660,6 +660,13 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp) > s->r_size_last_word = 0; > } This is a sysbus device -- when can it ever get unrealized ? -- PMM
On 8/19/21 4:21 PM, Peter Maydell wrote: > On Thu, 19 Aug 2021 at 15:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> Fixes: 35593573b25 ("hw/dma: Implement a Xilinx CSU DMA model") >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/dma/xlnx_csu_dma.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c >> index 797b4fed8f5..0c1c19cab5a 100644 >> --- a/hw/dma/xlnx_csu_dma.c >> +++ b/hw/dma/xlnx_csu_dma.c >> @@ -660,6 +660,13 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp) >> s->r_size_last_word = 0; >> } > > This is a sysbus device -- when can it ever get unrealized ? Alright. Then we should add an assertion if a SysBusDevice has a non-NULL unrealize() handler.
On Thu, 19 Aug 2021 at 15:40, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 8/19/21 4:21 PM, Peter Maydell wrote: > > On Thu, 19 Aug 2021 at 15:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> > >> Fixes: 35593573b25 ("hw/dma: Implement a Xilinx CSU DMA model") > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> hw/dma/xlnx_csu_dma.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c > >> index 797b4fed8f5..0c1c19cab5a 100644 > >> --- a/hw/dma/xlnx_csu_dma.c > >> +++ b/hw/dma/xlnx_csu_dma.c > >> @@ -660,6 +660,13 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp) > >> s->r_size_last_word = 0; > >> } > > > > This is a sysbus device -- when can it ever get unrealized ? > > Alright. Then we should add an assertion if a SysBusDevice has a > non-NULL unrealize() handler. There are a few corner cases where a sysbus device can be unrealized (eg if it is used as part of the implementation of a hotpluggable device like a PCI card). More generally, what are we trying to achieve here ? I definitely agree that our current situation wrt freeing of resources allocated during realize is liable to be a bit of a mess, but I'm not sure trying to patch individual cases one device at a time is likely to help. A comprehensive attack on the problem would probably involve: * documentation + what should go in instance_init and what in realize? + where should deallocation go? + if realize fails and it has already allocated something, who deallocates that and how? + are there cases which don't need to care about ever being unrealized, and how should those be written? + helpful checklist of common functions that need deinit, and ones that don't because they do refcounting * figuring out a test setup that would let us test the init->realize->unrealize->deinit cycle for all devices, not just hotpluggable ones. (We do init->deinit as part of the QOM introspection test; I'm not sure how much leakage of what kinds we catch that way.) * looking at how many of our existing devices fail that, and whether we can have an exclusion-list or something so at least new code has "get this right" tested, and maybe we can whittle down the exclusion-list over time (and we can prioritize the devices where this is actually a user visible bug or that get maintained) thanks -- PMM
© 2016 - 2024 Red Hat, Inc.