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 B50C73101A9 for ; Mon, 1 Jun 2026 16:41:54 +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=1780332116; cv=none; b=Zmv6pmOW7iYEaXR5e1zRljNQEcV4FlMiwHRI/l5W+viKko+16ShENJWgd0HuW7SQoK45pwcwlvnFA3h5OLN3qgtXTtoogCvKuDo5c0YQEJCbQWvAtd1nhoecUaoE7/mwkoA4517acGuqX/VWvAxHM1e2gLV8BrDQajkeUzmxJ1A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780332116; c=relaxed/simple; bh=T0NKGmw9QFRRXfIkfjpjprTtOQdF6COdXsYUeU0nUuQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=fC1Wwfj3MxaQWh6TMVAPitlhn4Fk5qcBKzB6KW3HbXpli+5WxMMUR67KnSv0jeSvm63IwYpgIPtLgxHl3bYalFLO3GEcYpWV9k89F9T4zIBFjzsKc8mOoVgtbRcf77rJulkDJ1Eg0dIXSfkRxAhcHSIvc0b/rkRACvcC0LsNSzU= 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=HdT2Fuxv; 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="HdT2Fuxv" 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 18F071BCA; Mon, 1 Jun 2026 09:41: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 A47D53F632; Mon, 1 Jun 2026 09:41:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1780332114; bh=T0NKGmw9QFRRXfIkfjpjprTtOQdF6COdXsYUeU0nUuQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HdT2FuxvvhiGm7Bqb1ZVtkHgG96z3UmGRwmdw5uvSJ94tas+HPsMhudMx1mmEe50v 3VSYFYKEEBu1DFb+0FFbi6FFN2tDPoXZqtEJcWu7TVG5/NuIdx2++elJ/kaAeh0SQ0 5F7rQ9f3FHsrDMk30/yjViG7rp2tj2jVlwNaL8XI= Date: Mon, 1 Jun 2026 17:41:50 +0100 From: Yeoreum Yun To: Suzuki K Poulose Cc: Leo Yan , coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mike.leach@arm.com, james.clark@linaro.org, alexander.shishkin@linux.intel.com, jie.gan@oss.qualcomm.com Subject: Re: [PATCH v7 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable Message-ID: References: <20260519154812.254884-1-yeoreum.yun@arm.com> <20260519154812.254884-10-yeoreum.yun@arm.com> <20260528143358.GF101133@e132581.arm.com> <20260528152633.GH101133@e132581.arm.com> <0fe7c30c-85e4-4205-930c-0c496d6263b8@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: <0fe7c30c-85e4-4205-930c-0c496d6263b8@arm.com> > On 28/05/2026 17:01, Yeoreum Yun wrote: > > > On Thu, May 28, 2026 at 03:43:40PM +0100, Yeoreum Yun wrote: > > > > > > [...] > > > > > > > > @@ -931,6 +919,18 @@ static int etm4_enable_perf(struct coresight_device *csdev, > > > > > if (ret) > > > > > goto err; > > > > > + /* > > > > > + * Set any selected configuration and preset. A zero configid means no > > > > > + * configuration active, preset = 0 means no preset selected. > > > > > + */ > > > > > + cfg_hash = ATTR_CFG_GET_FLD(attr, configid); > > > > > + if (cfg_hash) { > > > > > + preset = ATTR_CFG_GET_FLD(attr, preset); > > > > > + ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); > > > > > + if (ret) > > > > > + goto err; > > > > > + } > > > > > + > > > > > > > No. since preset overrides the "perf configuratoin" formerly but > > > > this code makes it vice versa. > > > > > > The above proposed change applies cfgfs after calling > > > etm4_parse_event_config(). This is just use preset to override the > > > config. Do I miss anything? > > > > Ah sorry. I've misread the code location that was my bad. > > > > > > > > > Also, cfg_hash and prest is also part of > > > > etm4_parse_event_config(), and it doesn't seem to good to separate > > > > cfgfs handling from that function. > > > > > > > > IMHO, It would be better to keep this as it is. > > > > > > I have another version to give a try. I'd leave to you and maintainers > > > to choose which is better. > > > > Funcionally, Code works. However, To be honest, the pairing between > > etm4_parse_event_config() and etm4_clean_event_config() feels a bit artificial to me. > > > > So here I have simply followed the principle that, > > if etm4_parse_event_config() fails, the configuration it touched should be > > cleaned up within that function; and if a failure happens after > > etm4_parse_event_config() has succeeded, the caller should perform the cleanup. > > > > Renaming etm4_parse_event_config() and splitting out the > > CSCFG-related handling as suggested would be possible, > > although I still feel it may not be strictly necessary. > > > > My preference would be to keep this as-is, but Suzuki, what do you think? > > I prefer the end result with Leo's patch applied. That kind of indicates > clearly that we need to cleanup something from the event config parsing > and keeps the disabling only once, rather than spreading it. > Okay. Then I'll apply with a little bit of modification. Thanks -- Sincerely, Yeoreum Yun