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

From: Suzuki K Poulose
Date: Wed Aug 16 2023 - 09:14:44 EST

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@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().

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@xxxxxxxxxx>
Cc: Anshuman Khandual <anshuman.khandual@xxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
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)
--
2.34.1





      if (csdev)
          coresight_remove_conns_sysfs_group(csdev);
  }


.