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