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 7B951C001B0 for ; Mon, 14 Aug 2023 22:58:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231467AbjHNW5n (ORCPT ); Mon, 14 Aug 2023 18:57:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35846 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231691AbjHNW5a (ORCPT ); Mon, 14 Aug 2023 18:57:30 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 17877127 for ; Mon, 14 Aug 2023 15:57:29 -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 048581063; Mon, 14 Aug 2023 15:58:11 -0700 (PDT) Received: from [10.57.90.230] (unknown [10.57.90.230]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 348573F762; Mon, 14 Aug 2023 15:57:27 -0700 (PDT) Message-ID: Date: Mon, 14 Aug 2023 23:57:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH 1/2] coresight: trbe: Fix TRBE potential sleep in atomic context To: hejunhao , 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, yangyicong@huawei.com, prime.zeng@hisilicon.com References: <20230814093813.19152-1-hejunhao3@huawei.com> <20230814093813.19152-2-hejunhao3@huawei.com> <37e36bde-fc70-6d2d-8dec-28e572f618cc@arm.com> <4049aee1-7f4d-5a54-ce27-7d802ae9616d@huawei.com> From: Suzuki K Poulose In-Reply-To: <4049aee1-7f4d-5a54-ce27-7d802ae9616d@huawei.com> 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 14/08/2023 14:32, hejunhao wrote: > Hi Suzuki > > > On 2023/8/14 18:34, Suzuki K Poulose wrote: >> Hi Junhao >> >> On 14/08/2023 10:38, Junhao He wrote: >>> smp_call_function_single() will allocate an IPI interrupt vector to >>> the target processor and send a function call request to the interrupt >>> vector. After the target processor receives the IPI interrupt, it will >>> execute arm_trbe_remove_coresight_cpu() call request in the interrupt >>> handler. >>> >>> According to the device_unregister() stack information, if other process >>> is useing the device, the down_write() may sleep, and trigger deadlocks >>> or unexpected errors. >>> >>>    arm_trbe_remove_coresight_cpu >>>      coresight_unregister >>>        device_unregister >>>          device_del >>>            kobject_del >>>              __kobject_del >>>                sysfs_remove_dir >>>                  kernfs_remove >>>                    down_write ---------> it may sleep >>> >>> Add a helper arm_trbe_disable_cpu() to disable TRBE precpu irq and reset >>> per TRBE. >>> Simply call arm_trbe_remove_coresight_cpu() directly without useing the >>> smp_call_function_single(), which is the same as registering the TRBE >>> coresight device. >>> >>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver") >>> Signed-off-by: Junhao He >>> --- >>>   drivers/hwtracing/coresight/coresight-trbe.c | 35 +++++++++++--------- >>>   1 file changed, 20 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c >>> b/drivers/hwtracing/coresight/coresight-trbe.c >>> index 7720619909d6..ce1e6f537b8d 100644 >>> --- a/drivers/hwtracing/coresight/coresight-trbe.c >>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c >>> @@ -1225,6 +1225,17 @@ static void arm_trbe_enable_cpu(void *info) >>>       enable_percpu_irq(drvdata->irq, IRQ_TYPE_NONE); >>>   } >>>   +static void arm_trbe_disable_cpu(void *info) >>> +{ >>> +    struct trbe_drvdata *drvdata = info; >>> +    struct trbe_cpudata *cpudata = this_cpu_ptr(drvdata->cpudata); >>> + >>> +    disable_percpu_irq(drvdata->irq); >>> +    trbe_reset_local(cpudata); >>> +    cpudata->drvdata = NULL; >>> +} >>> + >>> + >>>   static void arm_trbe_register_coresight_cpu(struct trbe_drvdata >>> *drvdata, int cpu) >>>   { >>>       struct trbe_cpudata *cpudata = per_cpu_ptr(drvdata->cpudata, cpu); >>> @@ -1326,18 +1337,12 @@ static void arm_trbe_probe_cpu(void *info) >>>       cpumask_clear_cpu(cpu, &drvdata->supported_cpus); >>>   } >>>   -static void arm_trbe_remove_coresight_cpu(void *info) >>> +static void arm_trbe_remove_coresight_cpu(struct trbe_drvdata >>> *drvdata, int cpu) >>>   { >>> -    int cpu = smp_processor_id(); >>> -    struct trbe_drvdata *drvdata = info; >>> -    struct trbe_cpudata *cpudata = per_cpu_ptr(drvdata->cpudata, cpu); >>>       struct coresight_device *trbe_csdev = >>> coresight_get_percpu_sink(cpu); >>>   -    disable_percpu_irq(drvdata->irq); >>> -    trbe_reset_local(cpudata); >>>       if (trbe_csdev) { >>>           coresight_unregister(trbe_csdev); >>> -        cpudata->drvdata = NULL; >>>           coresight_set_percpu_sink(cpu, NULL); >> >> I am a bit concerned about "resetting" the sink from a different CPU. >> Could we instead, schedule a delayed work to unregister the trbe_csdev? > > Yes, I will try to do that. > Sorry for my following questions. > As you mean, do we need to take the same care when setting the percpu sink > in the register trbe_csdev ? Apologies, having taken another look, we set the percpu_sink for a cpu outside smp_call_function(). So, I think your patch is fine. > > Best regards, > Junhao. > >> >> >>>       } >>>   } >>> @@ -1366,8 +1371,12 @@ static int arm_trbe_remove_coresight(struct >>> trbe_drvdata *drvdata) >>>   { >>>       int cpu; >>>   -    for_each_cpu(cpu, &drvdata->supported_cpus) >>> -        smp_call_function_single(cpu, arm_trbe_remove_coresight_cpu, >>> drvdata, 1); >>> +    for_each_cpu(cpu, &drvdata->supported_cpus) { >>> +        if (cpumask_test_cpu(cpu, &drvdata->supported_cpus)) >>> +            smp_call_function_single(cpu, arm_trbe_disable_cpu, >>> drvdata, 1); >>> +        if (cpumask_test_cpu(cpu, &drvdata->supported_cpus)) >>> +            arm_trbe_remove_coresight_cpu(drvdata, cpu); Do we need to test the cpu here in both places ? We already check that in the loop entry. The reason why we repeat the check during the probe, is to skip any CPUs that may have a TRBE not accessible. Suzuki >>> +    } >>>       free_percpu(drvdata->cpudata); >>>       return 0; >>>   } >>> @@ -1406,12 +1415,8 @@ static int arm_trbe_cpu_teardown(unsigned int >>> cpu, struct hlist_node *node) >>>   { >>>       struct trbe_drvdata *drvdata = hlist_entry_safe(node, struct >>> trbe_drvdata, hotplug_node); >>>   -    if (cpumask_test_cpu(cpu, &drvdata->supported_cpus)) { >>> -        struct trbe_cpudata *cpudata = per_cpu_ptr(drvdata->cpudata, >>> cpu); >>> - >>> -        disable_percpu_irq(drvdata->irq); >>> -        trbe_reset_local(cpudata); >>> -    } >>> +    if (cpumask_test_cpu(cpu, &drvdata->supported_cpus)) >>> +        arm_trbe_disable_cpu(drvdata); >>>       return 0; >>>   } >> >> >> . >> >