Messages in this thread | | | Subject | Re: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data resource | From | hejunhao <> | Date | Tue, 15 Aug 2023 19:38:17 +0800 |
| |
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@huawei.com> >> --- >> 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); >> } > > > . >
| |