| Message ID | 20210819141527.2821842-1-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/dma/xlnx_csu_dma: Fix ptimer resource leak | expand |
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
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);
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(+)