diff mbox series

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

Message ID 20230814093813.19152-3-hejunhao3@huawei.com
State New
Headers
Series Fix some issues with TRBE building as a module |

Commit Message

hejunhao Aug. 14, 2023, 9:38 a.m. UTC 
  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(-)

Comments

Suzuki K Poulose Aug. 14, 2023, 10:47 p.m. UTC | #1 
+ 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);
>   }
hejunhao Aug. 15, 2023, 11:38 a.m. UTC | #2 
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);
>>   }
>
>
> .
>
Suzuki K Poulose Aug. 16, 2023, 1:13 p.m. UTC | #3 
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)
Suzuki K Poulose Aug. 16, 2023, 1:58 p.m. UTC | #4 
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 mbox series

Patch

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);
 }