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.5 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, 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 2AA81C43460 for ; Mon, 26 Apr 2021 10:42:30 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 8144D61139 for ; Mon, 26 Apr 2021 10:42:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8144D61139 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=desiato.20200630; 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=A++XYpIftnuwGpv/XO1j9dFyIGf2usyNgd97ClXoX7M=; b=rEU1Bn9oYzuHEC2ta5daaaNMw dm64AGRwzVKctg2ptkhLtgoIoDPSqjn29kZRdOOxS1ERSjOqzd0TBU0SoW1jyyNdtWs0Z8aNa5X6s /9qew/OncrUpbFppdj1nPFmaQfxsu3ga+W951X/Xq7eIfsKk0Z9xKoJ9QPmquVgZgfW7JPiviYBzQ BQm2wrkhla90BKtcjhxCY7GrTgf+NVkvO4yXQ8bdgDxOhCGEsCVmYLgDmm6ecAysUXmqMX2FgHHUc 9djOX7VYkwO8ULHlHhD6nS70dKygf0SS8pCoK5yA9jqy+KU+GRFnTyQ6RVU9OBY3ITunNcTxRRRYM A9sjlDqSQ==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1layfk-007Qjv-9s; Mon, 26 Apr 2021 10:41:00 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1layfh-007QjV-1B for linux-arm-kernel@desiato.infradead.org; Mon, 26 Apr 2021 10:40:57 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Transfer-Encoding: Content-Type:In-Reply-To:MIME-Version:Date:Message-ID:From:References:Cc:To: Subject:Sender:Reply-To:Content-ID:Content-Description; bh=57JhCAVuyKm8eiVZ4R4DyVgW8h8Popzd/VW/nkh4eHw=; b=4mPSAONuY9yMcdlSA0ANHqG2V0 5w8C5CLPbv/IHBY++qEvV9uBs6QYNKicnBM+qoIaGW0PYFm5B82h+iP2GrWRtlwic/CwhHOmbLRrn dRpKELk2TnivzA5aZfeUpk4WBp5zH6k49nAOhZECtMEdW0fHOLOVsLIk86s7JLn55YEwSbxg2phoQ gUNw2J7UvtSJVST4KDudRDCMnMqbUr4ix36LyuQeTFUxvhfjhfMu5vlPaIC79bdD7dvyQT6Rg/9F9 cyMpc4fK67jy6+bacwjJtV8AtOPjxF1pV00ydnZwLIkFKFLPgPax8Oi9xXAFifT9IivAnhceonQ3f mVjreH1A==; Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1layfe-00FtzB-8M for linux-arm-kernel@lists.infradead.org; Mon, 26 Apr 2021 10:40:55 +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 614751FB; Mon, 26 Apr 2021 03:40:47 -0700 (PDT) Received: from [10.57.67.211] (unknown [10.57.67.211]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1F8E33F73B; Mon, 26 Apr 2021 03:40:45 -0700 (PDT) Subject: Re: [PATCH 1/4] coresight: tmc-etr: Advance buffer pointer in sync buffer. To: Daniel Kiss , mathieu.poirier@linaro.org, mike.leach@linaro.org, leo.yan@linaro.org, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org Cc: denik@google.com, Branislav Rankov References: <20210421120413.3110775-1-daniel.kiss@arm.com> <20210421120413.3110775-2-daniel.kiss@arm.com> From: Suzuki K Poulose Message-ID: Date: Mon, 26 Apr 2021 11:40:44 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <20210421120413.3110775-2-daniel.kiss@arm.com> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210426_034054_442944_37A17908 X-CRM114-Status: GOOD ( 24.22 ) 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/04/2021 13:04, Daniel Kiss wrote: > With polling the sync could called multiple times in a row. Without this > change the data will be overwritten at the beginning of the buffer. > > Signed-off-by: Daniel Kiss > Signed-off-by: Branislav Rankov > --- > drivers/hwtracing/coresight/coresight-tmc-etr.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index ea5027e397d02..dd19d1d1c3b38 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1442,7 +1442,7 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf, > { > long bytes; > long pg_idx, pg_offset; > - unsigned long head = etr_perf->head; > + unsigned long head; > char **dst_pages, *src_buf; > struct etr_buf *etr_buf = etr_perf->etr_buf; > > @@ -1465,7 +1465,7 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf, > bytes = tmc_etr_buf_get_data(etr_buf, src_offset, to_copy, > &src_buf); > if (WARN_ON_ONCE(bytes <= 0)) > - break; > + return; > bytes = min(bytes, (long)(PAGE_SIZE - pg_offset)); > > memcpy(dst_pages[pg_idx] + pg_offset, src_buf, bytes); > @@ -1483,6 +1483,7 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf, > /* Move source pointers */ > src_offset += bytes; > } > + etr_perf->head = (pg_idx << PAGE_SHIFT) + pg_offset; Looking at this patch, I feel the driver is doing a couple wrong things already. 1) We initialise etr_perf->head every time the ETR enable is called, irrespective of whether we actually try to enable the Hardware. e.g, etm_0 on -> .. -> enable_etr : etr_perf->head = enable_hw() emt_1 on -> ... -> enable_etr: etr_perf->head = already_enabled, skip enable_hw() etm_2 on -> ... -> enable_etr: etr_perf->head = already_enable, skip enable_hw()... This doesn't look correct as we don't know which handle is going to get the data. This looks pointless. 2) Even more problematic is where we copy the AUX buffer content to. As mentioned above, we don't know which handle is going to be the last one to consume and we have a "etr_perf->head" that came from one of the handles and the "pages" that came from the first handle which created a etr_perf buffer. In sync_perf_buffer() we copy the hardware buffers to the "pages" (say of handle_0) with "etr_perf->head" (which could be from any other handle, say handle_2) and then we could return the number of bytes copied, which then is used to update the last handle (could be say handle_3), where there is no actual data copied. To fix all of these issues, we must 1) Stop using etr_perf->head, and instead use the handle->head where we are called update_buffer on. 2) Keep track of the "pages" that belong to a given "handle" and then use those pages to copy the data to the current handle we are called to update the buffer on. Did I get this wrong ? Suzuki _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel