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 X-Spam-Level: X-Spam-Status: No, score=-15.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A8178C433E0 for ; Fri, 12 Feb 2021 05:44:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5EFFA64DD1 for ; Fri, 12 Feb 2021 05:44:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5EFFA64DD1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:References: To:Subject:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=+huPP9mnCuW9dlfTX6kC3jEP1JV9YZglbHiAa3MXbzY=; b=x59ikagwxvIV8cJv8umK28ENY UHH7M/SCgJ68diiuI+Um9joSazLM1JFK/GjeCVCFEcL7rVNfjGvoSFz9MFe8L2SWg6oUNNYJkBoBS sjpey0qnG6T13o8G1eVPG44XtN+fdekfc/CQflA2gszGGiHCQiGZZqcKvNRGY7QlhnikWuDIq4Vuw dS311uDl+7U/mE/Fyfx3RZr2ZF14HKeFS4uJFb9hovBs25sQ4o5epuWf0fNK8Dnbbfc03uJ1cSMdA 2JtTmpH/D9+n96xQJYH1KkiJ4A5NmAwutRm9Ob73D89TP+vRfhNazf00drUv2x9zgEDcewt61B1Ul d+s5XyYgg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lARED-0006qp-KG; Fri, 12 Feb 2021 05:42:53 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lAREA-0006qN-IV for linux-arm-kernel@lists.infradead.org; Fri, 12 Feb 2021 05:42:51 +0000 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 7533E113E; Thu, 11 Feb 2021 21:42:42 -0800 (PST) Received: from [192.168.0.130] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DABE33F719; Thu, 11 Feb 2021 21:42:39 -0800 (PST) From: Anshuman Khandual Subject: Re: [PATCH V3 11/14] coresight: sink: Add TRBE driver To: Mathieu Poirier References: <1611737738-1493-1-git-send-email-anshuman.khandual@arm.com> <1611737738-1493-12-git-send-email-anshuman.khandual@arm.com> <20210210190027.GC2186000@xps15> Message-ID: <9787ef82-9bd9-b3ec-b899-8e682dfa3971@arm.com> Date: Fri, 12 Feb 2021 11:13:01 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20210210190027.GC2186000@xps15> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210212_004250_756618_FADFDEAC X-CRM114-Status: GOOD ( 40.79 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: suzuki.poulose@arm.com, coresight@lists.linaro.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, lcherian@marvell.com, mike.leach@linaro.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2/11/21 12:30 AM, Mathieu Poirier wrote: > On Wed, Jan 27, 2021 at 02:25:35PM +0530, Anshuman Khandual wrote: >> Trace Buffer Extension (TRBE) implements a trace buffer per CPU which is >> accessible via the system registers. The TRBE supports different addressing >> modes including CPU virtual address and buffer modes including the circular >> buffer mode. The TRBE buffer is addressed by a base pointer (TRBBASER_EL1), >> an write pointer (TRBPTR_EL1) and a limit pointer (TRBLIMITR_EL1). But the >> access to the trace buffer could be prohibited by a higher exception level >> (EL3 or EL2), indicated by TRBIDR_EL1.P. The TRBE can also generate a CPU >> private interrupt (PPI) on address translation errors and when the buffer >> is full. Overall implementation here is inspired from the Arm SPE driver. >> >> Cc: Mathieu Poirier >> Cc: Mike Leach >> Cc: Suzuki K Poulose >> Signed-off-by: Anshuman Khandual >> --- >> Changes in V3: >> >> - Added new DT bindings document TRBE.yaml >> - Changed TRBLIMITR_TRIG_MODE_SHIFT from 2 to 3 >> - Dropped isb() from trbe_reset_local() >> - Dropped gap between (void *) and buf->trbe_base >> - Changed 'int' to 'unsigned int' in is_trbe_available() >> - Dropped unused function set_trbe_running(), set_trbe_virtual_mode(), >> set_trbe_enabled() and set_trbe_limit_pointer() >> - Changed get_trbe_flag_update(), is_trbe_programmable() and >> get_trbe_address_align() to accept TRBIDR value >> - Changed is_trbe_running(), is_trbe_abort(), is_trbe_wrap(), is_trbe_trg(), >> is_trbe_irq(), get_trbe_bsc() and get_trbe_ec() to accept TRBSR value >> - Dropped snapshot mode condition in arm_trbe_alloc_buffer() >> - Exit arm_trbe_init() when arm64_kernel_unmapped_at_el0() is enabled >> - Compute trbe_limit before trbe_write to get the updated handle >> - Added trbe_stop_and_truncate_event() >> - Dropped trbe_handle_fatal() >> >> Documentation/trace/coresight/coresight-trbe.rst | 39 + >> arch/arm64/include/asm/sysreg.h | 1 + >> drivers/hwtracing/coresight/Kconfig | 11 + >> drivers/hwtracing/coresight/Makefile | 1 + >> drivers/hwtracing/coresight/coresight-trbe.c | 1023 ++++++++++++++++++++++ >> drivers/hwtracing/coresight/coresight-trbe.h | 160 ++++ >> 6 files changed, 1235 insertions(+) >> create mode 100644 Documentation/trace/coresight/coresight-trbe.rst >> create mode 100644 drivers/hwtracing/coresight/coresight-trbe.c >> create mode 100644 drivers/hwtracing/coresight/coresight-trbe.h >> + > > [...] > >> +static void arm_trbe_probe_coresight_cpu(void *info) >> +{ >> + struct trbe_drvdata *drvdata = info; >> + struct coresight_desc desc = { 0 }; >> + int cpu = smp_processor_id(); >> + struct trbe_cpudata *cpudata = per_cpu_ptr(drvdata->cpudata, cpu); >> + struct coresight_device *trbe_csdev = per_cpu(csdev_sink, cpu); >> + u64 trbidr = read_sysreg_s(SYS_TRBIDR_EL1); >> + struct device *dev; >> + >> + if (WARN_ON(!cpudata)) >> + goto cpu_clear; > > There is already a check for this in arm_trbe_probe_coresight(), we couldn't be > here if there was a problem with the allocation. Right but just to be extra cautious. Do you really want this to be dropped ? > >> + >> + if (trbe_csdev) >> + return; > > Now that's a reason to have a WARN_ON(). If we are probing and a sink is > already present in this cpu's slot, something went seriously wrong and we should > be clear about it. Right, will add an WARN_ON(). > >> + >> + cpudata->cpu = smp_processor_id(); >> + cpudata->drvdata = drvdata; >> + dev = &cpudata->drvdata->pdev->dev; >> + >> + if (!is_trbe_available()) { >> + pr_err("TRBE is not implemented on cpu %d\n", cpudata->cpu); >> + goto cpu_clear; >> + } >> + >> + if (!is_trbe_programmable(trbidr)) { >> + pr_err("TRBE is owned in higher exception level on cpu %d\n", cpudata->cpu); >> + goto cpu_clear; >> + } >> + desc.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", DRVNAME, smp_processor_id()); > > We will end up with "arm_trbe0", "arm_trbe1" and so on in sysfs... Is the > "arm_" part absolutely needed? I think this should be like what we do for etmv3 > and etmv4 where only "etmX" shows up in sysfs. Okay, will drop arm_ here. IIRC this was originally trbeX where X is the cpu number but then ended up using DRVNAME as prefix. > >> + if (IS_ERR(desc.name)) >> + 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); >> + if (IS_ERR(trbe_csdev)) >> + goto cpu_clear; >> + >> + dev_set_drvdata(&trbe_csdev->dev, cpudata); >> + cpudata->trbe_dbm = get_trbe_flag_update(trbidr); >> + cpudata->trbe_align = 1ULL << get_trbe_address_align(trbidr); >> + if (cpudata->trbe_align > SZ_2K) { >> + pr_err("Unsupported alignment on cpu %d\n", cpudata->cpu); >> + goto cpu_clear; > > Here coresight_unregister() should be called. The other option is to call > coresight_register() when everything else is known to be fine, which is the > favoured approach. Okay, will change accordingly. > >> + } >> + per_cpu(csdev_sink, cpu) = trbe_csdev; >> + trbe_reset_local(); >> + enable_percpu_irq(drvdata->irq, IRQ_TYPE_NONE); >> + return; >> +cpu_clear: >> + cpumask_clear_cpu(cpudata->cpu, &cpudata->drvdata->supported_cpus); >> +} >> + >> +static void arm_trbe_remove_coresight_cpu(void *info) >> +{ >> + 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 = per_cpu(csdev_sink, cpu); >> + >> + if (trbe_csdev) { > > In what scenario do you see not having a trbe_csdev and still needing to disable > IRQs for the HW? If there is a such a case then a few lines of comment is > needed. > >> + coresight_unregister(trbe_csdev); >> + cpudata->drvdata = NULL; >> + per_cpu(csdev_sink, cpu) = NULL; >> + } >> + disable_percpu_irq(drvdata->irq); >> + trbe_reset_local(); > > Theoretically this code shouldn't run when the TRBE is enabled, because the CS > core will prevent that from happening. As sush disabling interrupts after > coresight_unregister() has been called and setting cpudata->drvdata to NULL > should be fine. But from an outsider's point of view it will look very bizarre. > Either write a comment to explain all that or call the above two before doing > the cleanup. Okay, will move them before the cleanup. > >> +} >> + >> +static int arm_trbe_probe_coresight(struct trbe_drvdata *drvdata) >> +{ >> + drvdata->cpudata = alloc_percpu(typeof(*drvdata->cpudata)); >> + if (IS_ERR(drvdata->cpudata)) >> + return PTR_ERR(drvdata->cpudata); > > As far as I can tell alloc_percpu() returns NULL on failure and nothing else. Sure, will change the return code as -ENOMEM when alloc_percpu() returns NULL. > >> + >> + arm_trbe_probe_coresight_cpu(drvdata); >> + smp_call_function_many(&drvdata->supported_cpus, arm_trbe_probe_coresight_cpu, drvdata, 1); > > The above two calls look racy to me. The executing process could be moved to > another CPU between the call to arm_trbe_probe_coresight_cpu() and > smp_call_function_many(), which would prevent the initialisation of the TRBE on > the new CPU to be done. I suggest using a for_each_cpu() loop where > smp_call_function_single() would be used. That way we are guaranteed all the > TRBEs will be initialised. Okay, will change. > >> + return 0; >> +} >> + >> +static int arm_trbe_remove_coresight(struct trbe_drvdata *drvdata) >> +{ >> + arm_trbe_remove_coresight_cpu(drvdata); >> + smp_call_function_many(&drvdata->supported_cpus, arm_trbe_remove_coresight_cpu, drvdata, 1); > > Same as above. Okay, will do. > > I'm out of time for today, more to come tomorrow. Okay. > > Mathieu - Anshuman _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel