Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4bb59de4-0fab-99f3-41d0-607d5085df05@arm.com>
Date:   Wed, 16 Aug 2023 14:58:57 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data
 resource
Content-Language: en-US
From:   Suzuki K Poulose <suzuki.poulose@arm.com>
To:     hejunhao <hejunhao3@huawei.com>, mike.leach@linaro.org,
        leo.yan@linaro.org, anshuman.khandual@arm.com,
        jonathan.cameron@huawei.com, James Clark <james.clark@arm.com>
Cc:     coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linuxarm@huawei.com,
        yangyicong@huawei.com, prime.zeng@hisilicon.com
References: <20230814093813.19152-1-hejunhao3@huawei.com>
 <20230814093813.19152-3-hejunhao3@huawei.com>
 <2b69bd4e-5ef4-16c7-f908-7c70187e12b6@arm.com>
 <68f16bb1-53ed-b8b9-812e-6d3be40a5bde@huawei.com>
 <a5a01257-f208-5dd8-bdb3-63c5578adf59@arm.com>
In-Reply-To: <a5a01257-f208-5dd8-bdb3-63c5578adf59@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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)