Re: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data resource

[hejunhao]

Hi, Suzuki


On 2023/8/15 6:47, Suzuki K Poulose wrote:
> + James Clark
>
> On 14/08/2023 10:38, Junhao He wrote:
> > Current the TRBE driver supports matching TRBE platform device through
> > id_table. The ACPI created a dummy TRBE platform device inside
> > drivers/perf/arm_pmu_acpi.c. So the TRBE platform driver will probe only
> > once and allocate just one TRBE platform data resource.
> >
> > If the system supports the TRBE feature, Each CPU in the systems can
> > have at least one TRBE present, and the coresight_unregister gets called
> > multiple times, once for each of them.
> > Therefore, when unregister TRBE coresight devices, the TRBE platform 
> > data
> > resource will multiple free in function coresight_unregister.
> >
> > root@localhost:# insmod coresight-trbe.ko
> > root@localhost:# rmmod coresight-trbe.ko
> > [  423.455932] ------------[ cut here ]------------
> > [  423.461987] WARNING: CPU: 1 PID: 0 at drivers/base/devres.c:1064 
> > devm_kfree+0x88/0x98
> > [  423.483821] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G           
> > O       6.5.0-rc4+ #1
> > [  423.505842] pstate: 614000c9 (nZCv daIF +PAN -UAO -TCO +DIT -SSBS 
> > BTYPE=--)
> > ...
> > [  423.601301] Call trace:
> > [  423.604202]  devm_kfree+0x88/0x98
> > [  423.608369]  coresight_release_platform_data+0xb8/0xe0 [coresight]
> > [  423.616589]  coresight_unregister+0x120/0x170 [coresight]
> > [  423.623533]  arm_trbe_remove_coresight_cpu+0x70/0xa0 [coresight_trbe]
> > [  423.631082]  __flush_smp_call_function_queue+0x1e4/0x4e0
> > [  423.637471] generic_smp_call_function_single_interrupt+0x1c/0x30
> > [  423.644796]  ipi_handler+0x90/0x278
> > [  423.648992]  handle_percpu_devid_irq+0x90/0x250
> > [  423.654636]  generic_handle_domain_irq+0x34/0x58
> > [  423.659786]  gic_handle_irq+0x12c/0x270
> > [  423.664039]  call_on_irq_stack+0x24/0x30
> > [  423.668452]  do_interrupt_handler+0x88/0x98
> > [  423.673027]  el1_interrupt+0x48/0xe8
> > [  423.677413]  el1h_64_irq_handler+0x18/0x28
> > [  423.681781]  el1h_64_irq+0x78/0x80
> > [  423.685550]  default_idle_call+0x5c/0x180
> > [  423.689855]  do_idle+0x25c/0x2c0
> > [  423.694196]  cpu_startup_entry+0x2c/0x40
> > [  423.698373]  secondary_start_kernel+0x144/0x188
> > [  423.703920]  __secondary_switched+0xb8/0xc0
> > [  423.708972] ---[ end trace 0000000000000000 ]---
> > [  423.729209] ------------[ cut here ]------------
> > ...
> > [  423.735217] WARNING: CPU: 2 PID: 40 at drivers/base/devres.c:1064 
> > devm_kfree+0x88/0x98
> > ...
> > [  424.012385] WARNING: CPU: 3 PID: 0 at drivers/base/devres.c:1064 
> > devm_kfree+0x88/0x98
> > ...
> >
> > This patch does the following:
> > 1.TRBE coresight devices do not need regular connections information, We
> >    can free connections resource when the nr_conns is valid.
> > 2.And we can ignore the free platform data resource, it will be
> >    automatically free in platform_driver_unregister().
>
> Do we need a Fixes tag here ?

Yes, I will do that.

> > Signed-off-by: Junhao He <hejunhao3@xxxxxxxxxx>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c 
> > b/drivers/hwtracing/coresight/coresight-core.c
> > index 118fcf27854d..c6f7889d1b4d 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -1555,9 +1555,10 @@ void coresight_release_platform_data(struct 
> > coresight_device *csdev,
> >           conns[i]->dest_fwnode = NULL;
> >           devm_kfree(dev, conns[i]);
> >       }
> > -    devm_kfree(dev, pdata->out_conns);
> > -    devm_kfree(dev, pdata->in_conns);
> > -    devm_kfree(dev, pdata);
> > +    if (pdata->nr_outconns)
> > +        devm_kfree(dev, pdata->out_conns);
> > +    if (pdata->nr_inconns)
> > +        devm_kfree(dev, pdata->in_conns);
>
> These allocations are made on the parent device and that
> may never get unregistered (e.g., AMBA device, platform device,
> stay forever, even when the "coresight" modules are unloaded).
> Thus the memory will be left unused, literally leaking.
> This specific devm_kfree() was added to fix that. May be we should fix
> this in the TRBE driver to use separate pdata for the TRBE device
> instances.
>
> Suzuki

If we fix this with minimal changes, I think it is possible to add a check
and not free pdata if it is TRBE?

    if (csdev->subtype.sink_subtype != 
CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM)
        devm_kfree(dev, pdata);

Then free pdata in the end of arm_trbe_remove_coresight().

> >       if (csdev)
> >           coresight_remove_conns_sysfs_group(csdev);
> >   }
>
>
> .