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=-16.7 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=ham 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 D1E61C433F5 for ; Tue, 21 Sep 2021 21:33:37 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 A5CB661159 for ; Tue, 21 Sep 2021 21:33:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A5CB661159 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:Cc:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=PZ/eT7S5nxS7O29NoDVKMa3TMwWXOqWm5Ccd0v5lZL4=; b=pPB3OWlAkOU4AYbf2R4x1fWQvQ Jh9HeLhU2ys/6mZCBeN8jzeAhtgoMMECn8r8IRI4GoYaCUomWn4Q2M95wIGwa5ZM0w6C5qzlNJKvB b2MpUt+7yQaZeNh20yOpZ2WIuzLegQZi0G3m0mVI1vGPfvExafcaA21xfGpZhN/FDPrHF1vRZLROo 1RBLPBYac+TnUdxNZdEEInJwhsxUNE+ESiiCj99ZzsMPztPYpjl783vstVch5n9lnVG7jeG/i2mub JIk3P67RRQZwAzP+Cu4MM4e1/eQ/7ugV0LYQwp/iuqTlq/KoxJx3R5J4PvvcLLRsMSxVLbXy6stxK JQKnwZJA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mSnMN-005wSe-KC; Tue, 21 Sep 2021 21:31:28 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mSnKP-005vcZ-Dt for linux-arm-kernel@lists.infradead.org; Tue, 21 Sep 2021 21:29:27 +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 A2C97113E; Tue, 21 Sep 2021 14:29:23 -0700 (PDT) Received: from [10.57.95.67] (unknown [10.57.95.67]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1A3923F40C; Tue, 21 Sep 2021 14:29:21 -0700 (PDT) Subject: Re: [PATCH v3 06/10] coresight: trbe: Fix handling of spurious interrupts To: Mathieu Poirier Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, anshuman.khandual@arm.com, mike.leach@linaro.org, leo.yan@linaro.org, tamas.zsoldos@arm.com, jinlmao@qti.qualcomm.com, al.grant@arm.com, denik@google.com References: <20210914102641.1852544-1-suzuki.poulose@arm.com> <20210914102641.1852544-7-suzuki.poulose@arm.com> <20210921172439.GC2059841@p14s> From: Suzuki K Poulose Message-ID: <5f5cacfc-984f-c12a-8a70-480c0d5bc964@arm.com> Date: Tue, 21 Sep 2021 22:29:20 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20210921172439.GC2059841@p14s> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210921_142925_627330_730D7A86 X-CRM114-Status: GOOD ( 32.17 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 21/09/2021 18:24, Mathieu Poirier wrote: > On Tue, Sep 14, 2021 at 11:26:37AM +0100, Suzuki K Poulose wrote: >> On a spurious IRQ, right now we disable the TRBE and then re-enable >> it back, resetting the "buffer" pointers(i.e BASE, LIMIT and more >> importantly WRITE) to the original pointers from the AUX handle. >> This implies that we overwrite any trace that was written so far, >> (by overwriting TRBPTR) while we should have ignored the IRQ. >> >> This patch cleans the behavior, by only stopping the TRBE if the >> IRQ was indeed raised, as we can read the TRBSR without stopping >> the TRBE (Only writes to the TRBSR requires the TRBE disabled). >> And also, on detecting a spurious IRQ after examining the TRBSR, >> we simply re-enable the TRBE without touching the other parameters. >> >> Cc: Anshuman Khandual >> Cc: Mathieu Poirier >> Cc: Mike Leach >> Cc: Leo Yan >> Signed-off-by: Suzuki K Poulose >> --- >> drivers/hwtracing/coresight/coresight-trbe.c | 30 ++++++++++---------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c >> index 5297b11f26b7..de99dd0aecd3 100644 >> --- a/drivers/hwtracing/coresight/coresight-trbe.c >> +++ b/drivers/hwtracing/coresight/coresight-trbe.c >> @@ -677,16 +677,16 @@ static int arm_trbe_disable(struct coresight_device *csdev) >> >> static void trbe_handle_spurious(struct perf_output_handle *handle) >> { >> - struct trbe_buf *buf = etm_perf_sink_config(handle); >> + u64 limitr = read_sysreg_s(SYS_TRBLIMITR_EL1); >> >> - buf->trbe_limit = compute_trbe_buffer_limit(handle); >> - buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf); >> - if (buf->trbe_limit == buf->trbe_base) { >> - trbe_drain_and_disable_local(); >> - perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED); >> - return; >> - } >> - trbe_enable_hw(buf); >> + /* >> + * If the IRQ was spurious, simply re-enable the TRBE >> + * back without modifying the buffer parameters to >> + * retain the trace collected so far. >> + */ >> + limitr |= TRBLIMITR_ENABLE; >> + write_sysreg_s(limitr, SYS_TRBLIMITR_EL1); >> + isb(); > > I understand (and agree with) this part of the patch... > >> } >> >> static void trbe_handle_overflow(struct perf_output_handle *handle) >> @@ -759,12 +759,7 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev) >> enum trbe_fault_action act; >> u64 status; >> >> - /* >> - * Ensure the trace is visible to the CPUs and >> - * any external aborts have been resolved. >> - */ >> - trbe_drain_and_disable_local(); >> - >> + /* Reads to TRBSR_EL1 is fine when TRBE is active */ >> status = read_sysreg_s(SYS_TRBSR_EL1); >> /* >> * If the pending IRQ was handled by update_buffer callback [0] See below >> @@ -773,6 +768,11 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev) >> if (!is_trbe_irq(status)) >> return IRQ_NONE; >> >> + /* >> + * Ensure the trace is visible to the CPUs and >> + * any external aborts have been resolved. >> + */ >> + trbe_drain_and_disable_local(); > > But not this part... I can see why you'd want to move this after the check for > is_trbe_irq(), but not how it relates to spurious interrupts. To me it seems > like it is addressing another issue. If those code snippets are related then a > good dose of comments is missing. This step is to make sure that we stop the TRBE only when there was really something to process. (i.e, TRBSR indicates an IRQ was raised). Also, there is a comment [0] above, for handling a case where the TRBE event was consumed by the "update_buffer()" due to a race with IRQ handler. Thus we stop the TRBE only when we need to analyse the cause and take an action. I agree there is a bit of disconnect. I can think of the following options: - Split the patch to 2. with 1. Don't stop the trbe if there is no IRQ (the bit explained above) 2. Don't reset the TRBE ptrs on spurious IRQ OR - Add the above comment to the section. The commit description has a hint, "This patch cleans the behavior, by only stopping the TRBE if the IRQ was indeed raised", but I agree that the code could be documented too. Let me know what you think. Thanks for the review. Suzuki _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel