lkml.org 
[lkml]   [2023]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data resource
From
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)

\
 
 \ /
  Last update: 2023-08-16 16:01    [W:1.917 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site