Messages in this thread Patch in this message | | | Date | Wed, 16 Aug 2023 14:58:57 +0100 | Subject | Re: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data resource | From | Suzuki K Poulose <> |
| |
On 16/08/2023 14:13, Suzuki K Poulose wrote: > On 15/08/2023 12:38, hejunhao wrote: >> 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(). > > It is much nicer to do something like : > > > --8>-- > > > coresight: trbe: Allocate platform data per device > > Coresight TRBE driver shares a single platform data (which is empty btw). > However, with the commit 4e8fe7e5c3a5 > ("coresight: Store pointers to connections rather than an array of them") > the coresight core would free up the pdata, resulting in multiple attempts > to free the same pdata for TRBE instances. Fix this by allocating a > pdata per > coresight_device. > > Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver") > Reported-by: Junhao He <hejunhao3@huawei.com> > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > drivers/hwtracing/coresight/coresight-trbe.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c > b/drivers/hwtracing/coresight/coresight-trbe.c > index 025f70adee47..fbab2bb4db38 100644 > --- a/drivers/hwtracing/coresight/coresight-trbe.c > +++ b/drivers/hwtracing/coresight/coresight-trbe.c > @@ -1255,10 +1255,17 @@ static void > arm_trbe_register_coresight_cpu(struct trbe_drvdata *drvdata, int cp > if (!desc.name) > goto cpu_clear;
---8>--
> > + /* > + * TRBE doesn't have any connections described via firmware. Instead > + * we register the trbe instance as per-cpu sink. > + */
Ignore the comments above ^. I have tested this locally and works for me. Please could you confirm if this solves the problem for you ?
Kind regards Suzuki
> + desc.pdata = coresight_get_platform_data(dev); > + if (IS_ERR(desc.pdata)) > + goto cpu_clear; > + > desc.type = CORESIGHT_DEV_TYPE_SINK; > desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM; > desc.ops = &arm_trbe_cs_ops; > - desc.pdata = dev_get_platdata(dev); > desc.groups = arm_trbe_groups; > desc.dev = dev; > trbe_csdev = coresight_register(&desc); > @@ -1482,7 +1489,6 @@ static void arm_trbe_remove_irq(struct > trbe_drvdata *drvdata) > > static int arm_trbe_device_probe(struct platform_device *pdev) > { > - struct coresight_platform_data *pdata; > struct trbe_drvdata *drvdata; > struct device *dev = &pdev->dev; > int ret; > @@ -1497,12 +1503,7 @@ static int arm_trbe_device_probe(struct > platform_device *pdev) > if (!drvdata) > return -ENOMEM; > > - pdata = coresight_get_platform_data(dev); > - if (IS_ERR(pdata)) > - return PTR_ERR(pdata); > - > dev_set_drvdata(dev, drvdata); > - dev->platform_data = pdata; > drvdata->pdev = pdev; > ret = arm_trbe_probe_irq(pdev, drvdata); > if (ret)
| |