[PATCH] hw/dma/xlnx_csu_dma: Fix ptimer resource leak

[PATCH] hw/dma/xlnx_csu_dma: Fix ptimer resource leak

[Philippe Mathieu-Daudé]

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

Re: [PATCH] hw/dma/xlnx_csu_dma: Fix ptimer resource leak

[Peter Maydell]

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

Re: [PATCH] hw/dma/xlnx_csu_dma: Fix ptimer resource leak

[Philippe Mathieu-Daudé]

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.

Re: [PATCH] hw/dma/xlnx_csu_dma: Fix ptimer resource leak

[Peter Maydell]

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