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 37A1138A728 for ; Wed, 8 Apr 2026 09:17:56 +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=1775639878; cv=none; b=MhNz6HbvUbGqV5maeMEtskzOJ9E7am7mP7LwOZmgf+iOS/r4oRySXOT7FWzVHjLECQ2RgT6AGzL71KKhwYTMN9aFujQnYtrFnfwNyoUt4Bm5rWD+G5FthwevCBk4AC056dE7oZRmhd2ZzeuTrm4NgL52Q6U9sCVeU0JXIZpI9js= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775639878; c=relaxed/simple; bh=qvvlclEr5+IMumWBNAfQzto/zRrwMeMeoVde3/6P+vo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=NDIWZUPyLY7gAB1Re+na8QM6T+H+ZD50yNx78Lktg3MxjtuoN1kM8AOwwpJW6Ay6JvfJyiI+1cd67VuPWQxAfZutehPs5GWrzFg7sKuiW9RawabWLRaqL/oc8rADm09Ctaf8PFNLPNFYl0ZBW8v3mrp8+9cC6amfcUIqvO5fVcQ= 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=ky7RG1Tw; 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="ky7RG1Tw" 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 DDB4F293B; Wed, 8 Apr 2026 02:17:49 -0700 (PDT) Received: from e129823.arm.com (e129823.arm.com [10.1.197.6]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 55DB63F641; Wed, 8 Apr 2026 02:17:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1775639875; bh=qvvlclEr5+IMumWBNAfQzto/zRrwMeMeoVde3/6P+vo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ky7RG1TwfRJDtYeTAvPcsIRBA1uKZw3p1Yhx0bnSRpEKvHNrVKCPsW5ECXdQNtlA2 3r5CBMrk/PFdq7sKu+CtxmnIaB2Gl+qzEMLnkRG8qDDmbtCNNvknR2hggNT/Y0nAoC rUSpqAbiBs4QtZYTxklvplWhwsVyzhFnBH7I3WeA= Date: Wed, 8 Apr 2026 10:17:51 +0100 From: Yeoreum Yun To: Leo Yan Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, suzuki.poulose@arm.com, mike.leach@linaro.org, james.clark@linaro.org, alexander.shishkin@linux.intel.com Subject: Re: [PATCH 1/2] coresight: etm4x: fix inconsistencies with sysfs configration Message-ID: References: <20260317181705.2456271-1-yeoreum.yun@arm.com> <20260317181705.2456271-2-yeoreum.yun@arm.com> <20260407143028.GM356832@e132581.arm.com> 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: <20260407143028.GM356832@e132581.arm.com> Hi Leo, > On Tue, Mar 17, 2026 at 06:17:04PM +0000, Yeoreum Yun wrote: > > The current ETM4x configuration via sysfs can lead to the following > > inconsistencies: > > > > - If a configuration is modified via sysfs while a perf session is > > active, the running configuration may differ between before > > a sched-out and after a subsequent sched-in. > > > > - Once a perf session is enabled, some read-only register fields > > (e.g., TRCSSCSR) may not be reported correctly, > > because drvdata->config is cleared while enabling with perf mode, > > even though the information was previously read via etm4_init_arch_data(). > > > > To resolve these inconsistencies, the configuration should be separated into: > > > > - active_config, which represents the currently applied configuration > > - config, which stores the settings configured via sysfs. > > > > Signed-off-by: Yeoreum Yun > > --- > > .../hwtracing/coresight/coresight-etm4x-cfg.c | 2 +- > > .../coresight/coresight-etm4x-core.c | 45 +++++++++++-------- > > drivers/hwtracing/coresight/coresight-etm4x.h | 2 + > > 3 files changed, 30 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c > > index c302072b293a..84213d40d1ae 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c > > @@ -47,7 +47,7 @@ static int etm4_cfg_map_reg_offset(struct etmv4_drvdata *drvdata, > > struct cscfg_regval_csdev *reg_csdev, u32 offset) > > { > > int err = -EINVAL, idx; > > - struct etmv4_config *drvcfg = &drvdata->config; > > + struct etmv4_config *drvcfg = &drvdata->active_config; > > I'd suggest we leave out complex cfg things, we can refactor it > later. > > In this series, let us first separate active_config and config, and > keep using drvdata->config to save complex cfg ? Yes. but I think we should include the separation of ss_status in this patchset. > > > u32 off_mask; > > > > if (((offset >= TRCEVENTCTL0R) && (offset <= TRCVIPCSSCTLR)) || > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > index d565a73f0042..c552129c4a0c 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > @@ -88,9 +88,11 @@ static int etm4_probe_cpu(unsigned int cpu); > > */ > > static bool etm4x_sspcicrn_present(struct etmv4_drvdata *drvdata, int n) > > { > > + struct etmv4_config *config = &drvdata->active_config; > > + > > return (n < drvdata->nr_ss_cmp) && > > drvdata->nr_pe && > > - (drvdata->config.ss_status[n] & TRCSSCSRn_PC); > > + (config->ss_status[n] & TRCSSCSRn_PC); > > As Suzuki suggested in another reply, we need to extract capabilities > into a separate structure. I'd also extract status related registers > into a new structure: > > struct etm4_cap { > int nr_ss_cmp; > bool pe_comparator; // TRCSSCSRn.PC > bool dv_comparator; // TRCSSCSRn.DV > bool da_comparator; // TRCSSCSRn.DA > bool inst_comparator; // TRCSSCSRn.INST > > int ns_ex_level; > int nr_pe; > int nr_pe_cmp; > int nr_resource; > ... > } > > struct etm4_status_reg { > u32 ss_status[ETM_MAX_SS_CMP]; > u32 cntr_val[ETMv4_MAX_CNTR]; > } Thanks. I'll take with this as reference. > > [...] > > > @@ -911,14 +915,17 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa > > > > /* enable any config activated by configfs */ > > cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset); > > + > > + raw_spin_lock(&drvdata->spinlock); > > + > > + drvdata->active_config = drvdata->config; > > This is not an issue introduced by this patch, but we might need to > consider to copy active config until it has acquired SYSFS mode. > Otherwise, it might update config here but will disturb a perf session > has been running. Good catch. I think we should access "active_config" after taking a mode. That means cscfg_csdev_enable/disable_active_config() should be called after/before taking mode like PERF session otherwise active_config could be a race if perf and sysfs session are enabled parallely. > > > @@ -2246,7 +2254,8 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg) > > if (!desc.name) > > return -ENOMEM; > > > > - etm4_set_default(&drvdata->config); > > + etm4_set_default(&drvdata->active_config); > > Should we set default values to drvdata->config ? > > My understanding is drvdata->active_config would be always set at the > runtime, but "drvdata->config" should be initialized properly so it > can be consumed by sysfs knobs. Right. I've only thought about perf case not a sysfs knobs for this. Thanks. -- Sincerely, Yeoreum Yun