From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7A391C04FE2 for ; Wed, 16 Aug 2023 13:59:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245740AbjHPN7O (ORCPT ); Wed, 16 Aug 2023 09:59:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59014 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S245755AbjHPN7E (ORCPT ); Wed, 16 Aug 2023 09:59:04 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id DB513271D for ; Wed, 16 Aug 2023 06:59:00 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C1F92D75; Wed, 16 Aug 2023 06:59:41 -0700 (PDT) Received: from [10.1.197.1] (ewhatever.cambridge.arm.com [10.1.197.1]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EF0D73F762; Wed, 16 Aug 2023 06:58:58 -0700 (PDT) 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 To: hejunhao , mike.leach@linaro.org, leo.yan@linaro.org, anshuman.khandual@arm.com, jonathan.cameron@huawei.com, James Clark 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> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> --- >>>>   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 > Cc: Anshuman Khandual > Signed-off-by: Suzuki K Poulose > --- >  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)