From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id C23CF47D440 for ; Wed, 12 Aug 2026 16:45:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786553138; cv=none; b=ScVFDUxqtx5FJgp6kCBD3NgP6h2g7biyleGl1I1TqZz7vsa/b09FueU6KCR4QNt8/5EaTJTaUeYbhwZUz6kOIjmbMjgieNBbG36rZGF7fHkj37uhKGJXoNdYSLze0a0sfaXTB3vp1W5Upip62If5X4ReEEzJVtGMNsm9XAhM7VU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786553138; c=relaxed/simple; bh=kZfrWbpyWg25XOFCqRRGcIF1rCmQ4eG3ydpCkOGHodI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=MXWeCoLuagqxyTrSsJOb5cKN9S5qoATH/ub8PEXn4JfGuNmxFkNQ8BxEX5H9lQ9aLvMeUEMYzwGnuSCMpCMuJ10c9h3PSkCBLhVLb1Nh6HBmdQsLkooYfynb2M9pcMA9NrP+B9GFSgtAPpATpCRjAJ6soEXeLKKJMPNyKwGJEh4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=oOux6qhY; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="oOux6qhY" 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 DF77F1682; Wed, 12 Aug 2026 09:45:30 -0700 (PDT) Received: from localhost (unknown [10.2.196.114]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7BA1D3F632; Wed, 12 Aug 2026 09:45:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1786553134; bh=kZfrWbpyWg25XOFCqRRGcIF1rCmQ4eG3ydpCkOGHodI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=oOux6qhYWyqRR6AQNI3mpag4wcBPehmp/ImE64d+pwI7lYma08H/zuwvUthOA3I0x oiWrr2gANwwy90HJuPcvWdNY45AO+U02tO2oekuqeMCW+xElgqsOpMBqhXxVPA6qC9 jeth4bANAMhv7i/gk2ktctLrD9lxzTQqp4iLWlOk= Date: Wed, 12 Aug 2026 17:45:32 +0100 From: Leo Yan To: James Clark Cc: Suzuki K Poulose , Mike Leach , Suyash Mahar , Yeoreum Yun , Greg Kroah-Hartman , Qi Liu , Junhao He , coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Jonathan Cameron , sashiko-bot Subject: Re: [PATCH v3 1/8] coresight: tmc-etr: Don't stop Perf cleanup for active sysfs reads Message-ID: <20260812164532.GA313579@e132581.arm.com> References: <20260728-james-cs-multiple-per-threads-v3-0-6aee7579f1dc@linaro.org> <20260728-james-cs-multiple-per-threads-v3-1-6aee7579f1dc@linaro.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260728-james-cs-multiple-per-threads-v3-1-6aee7579f1dc@linaro.org> On Tue, Jul 28, 2026 at 04:00:13PM +0100, James Clark wrote: > The linked fixes commit deliberately allows reads of an old sysfs buffer > while in Perf mode because they are separate software buffers. However > it didn't modify tmc_disable_etr_sink() to match this relaxation. The > result is that when a Perf event ends while the sysfs buffer is being > read, clean up will be skipped. > > Fix it by ignoring the sysfs_reading flag unless the active session is > a sysfs one. I am not clear why this is relevant to per-thread mode, maybe it would be clear for me after reading other patches. For this patch self, I think the issue comes from the sink buffer's life time - SYSFS mode's buffer has longer life time than a sysfs session, it is designed to allow reading sysfs trace data after the sysfs session, or even during a perf session. So the code gets complex for handling these cases. I really think we should have a correct life time (or state machine) for sysfs buffer, something like: Operations | ETR sysfs buffer state machine ----------------------------------+----------------------------------------- | INVALID: Init state, buffer unallocated echo 1 > .../tmc_etr0/enable_sink | READY: user can read zeroed data echo 1 > .../etm0/enable_source | BUSY: user cannot read as hardware is | writing data to buffer echo 0 > .../etm0/enable_source | READY: trace is stopped, user can read | trace data echo 0 > .../tmc_etr0/enable_sink | INVALID: buffer has been released With the state machine's help, we might even don't need to bother what the sink device's mode is. > When sysfs and Perf share the same memory in ETR_MODE_RESRV mode, a new > Perf session needs to overwrite an old inactive sysfs session by zeroing > len. This avoids sysfs from reading stale data because it has a separate > set of offsets in its etr_buf struct, even if that's backed by the same > memory as the Perf one. This can be naturally resolved if we have a state machine above? [...] > +static bool tmc_perf_sysfs_shared(struct tmc_drvdata *drvdata, > + struct etr_buf *perf_buf) > +{ > + /* In ETR_MODE_RESRV mode, sysfs and Perf share the same memory. */ > + return perf_buf && > + drvdata->sysfs_buf && > + drvdata->sysfs_buf->mode == ETR_MODE_RESRV && > + perf_buf->mode == ETR_MODE_RESRV; > +} > + > static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, > struct coresight_path *path) > { > @@ -1772,6 +1782,18 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, > goto unlock_out; > } > > + /* > + * Don't use if it's shared and being read by sysfs. Sysfs may only > + * start reading (the cleared zero length buffer) after the first > + * tmc_enable_etr_sink_perf(), which changes the result of this check, > + * so it should only be done once. > + */ > + if ((drvdata->sysfs_reading && > + tmc_perf_sysfs_shared(drvdata, etr_perf->etr_buf))) { > + rc = -EBUSY; > + goto unlock_out; > + } This is quite tricky. If someone reads the sysfs entry in the middle of a perf session, and the traced task is migrated to a different CPU or wakes up, the sink device may need to be re-enabled, however the sink enabling could fail due to the concurrent sysfs read here. Thanks, Leo