Message ID | 20230814093813.19152-3-hejunhao3@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b824:0:b0:3f2:4152:657d with SMTP id z4csp2643696vqi; Mon, 14 Aug 2023 03:19:34 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE7dHMq+VaQxpFeBOUTFmZq7WP9z0sIQDDoiLsvIl8nKUe6ed14HLPa49kXUGiFPwgeedHu X-Received: by 2002:a05:6870:b69e:b0:1b0:805:8678 with SMTP id cy30-20020a056870b69e00b001b008058678mr9944676oab.24.1692008373754; Mon, 14 Aug 2023 03:19:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1692008373; cv=none; d=google.com; s=arc-20160816; b=kYTGR5a++jMe7XANZe3rl3B56fdn0+Y6G+gZx5sfMM8DzVTQoZe3rcEXQP8cJvQFkU izVPViDHMWaEmw/aGRPUOXyg4vVou86H1MEY5n/79itrAGpbnXZ0S+itqrdNtI/zhMtA touNt7FvhT06rIdLfmibYCmt9/1RGD5NRNQboUldpe9SNnxEzlehHVr/MNO2/2/4OhWX wiIMc6keOVFJPHLawvjBP/LNZkZ5KlOS1PAI+vXjv8Z5IuA/t40knzKmhryAGRaHmLZg oWHiFhkVtrwWPMovHB32K/IIMDNfM5vV2GTyGm/CYeAkjNfIkxD2Fs+VhXUk+3Gjaze6 Abjw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=MhEMqdG5lCKbLXlT/m7PZVU2oqIAbfM1D05YRKq9BiU=; fh=qw43QZAXcNcl7h9B3MdyBVec7mySpMzzcuDhZUv6g+o=; b=WB5dtv5AJTEzS+6OfHP9QSCobeo8XnShrVWYDxT+akBUcMjSlXqwLzk1hXgtaZW5zd W1QlMEW2kFq2mzX0pUZ5UXNHit7w+npmgpSaynb6J2z8XCOhcudK1XJ/0R+iVcnFiRkO KM0IzJFnTLTrMdT46isS8i5j9C7KW9o9hkjOwx5cTwBwba24Jju4lD2bUn9ysBl7Njar 5/1pCVmgsSEyt2K9kUWq5Dv1tUMoY8mb3kxLE2rCfyRmROaXsBratITPqV3LwSbpdXUK jxx77YGGuYlWsIumgQ6LCQ6zftzPIkwuGyZp4CIxlcuOnQmiZFb/SC6lPZaPt5rzjbfM MXcg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id h16-20020a17090adb9000b0026841a42bffsi7865241pjv.139.2023.08.14.03.19.20; Mon, 14 Aug 2023 03:19:33 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234025AbjHNJlY (ORCPT <rfc822;274620705z@gmail.com> + 99 others); Mon, 14 Aug 2023 05:41:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49838 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234326AbjHNJlI (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 14 Aug 2023 05:41:08 -0400 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5ABA5E5F for <linux-kernel@vger.kernel.org>; Mon, 14 Aug 2023 02:41:07 -0700 (PDT) Received: from dggpeml500002.china.huawei.com (unknown [172.30.72.54]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4RPTpn1fN5z2BdKG; Mon, 14 Aug 2023 17:38:09 +0800 (CST) Received: from localhost.localdomain (10.69.192.56) by dggpeml500002.china.huawei.com (7.185.36.158) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Mon, 14 Aug 2023 17:41:04 +0800 From: Junhao He <hejunhao3@huawei.com> To: <suzuki.poulose@arm.com>, <mike.leach@linaro.org>, <leo.yan@linaro.org>, <anshuman.khandual@arm.com>, <jonathan.cameron@huawei.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>, <hejunhao3@huawei.com> Subject: [PATCH 2/2] coresight: core: Fix multiple free TRBE platform data resource Date: Mon, 14 Aug 2023 17:38:13 +0800 Message-ID: <20230814093813.19152-3-hejunhao3@huawei.com> X-Mailer: git-send-email 2.33.0 In-Reply-To: <20230814093813.19152-1-hejunhao3@huawei.com> References: <20230814093813.19152-1-hejunhao3@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.69.192.56] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpeml500002.china.huawei.com (7.185.36.158) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1774199372513852733 X-GMAIL-MSGID: 1774199372513852733 |
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
+ 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); }