| Message ID | 20230814093813.19152-3-hejunhao3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some issues with TRBE building as a module | expand |
+ 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 ? > > 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 (csdev) > coresight_remove_conns_sysfs_group(csdev); > }
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); >> } > > > . >
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; + /* + * TRBE doesn't have any connections described via firmware. Instead + * we register the trbe instance as per-cpu sink. + */ + 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)
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)
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); if (csdev) coresight_remove_conns_sysfs_group(csdev); }
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(). Signed-off-by: Junhao He <hejunhao3@huawei.com> --- drivers/hwtracing/coresight/coresight-core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)