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 7CEF840756C for ; Thu, 28 May 2026 16:01:26 +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=1779984088; cv=none; b=LjiHSyakn6PWSz1H5MchEVzqA8isCxrSmiTva6pXMdT4kLqTElyGGdN36QvIEY7CmAlWmW0YTKYOibwjjvXFuVj6XR3TR1oU0+Z8wiYfhqQrkBZuLErlbBf1fyh3FfwM3L1WxivyW9wH2dsGvv6wtpJmbmAnHo1ucpEuVe5pWBE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779984088; c=relaxed/simple; bh=Pb9KUS39SJ5eL/Q/btXVm8CCz+Epp99Lk53Gb8UqVPk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=CnpL3SPBEDQnvmO3AI74YBHu/4a6nl40I3LdUUNXz7z+gTvHaR/klCjZ04WvnEE/nWl9Hdj8zu9NgkQ3zgijIFSJu/0UFqdrT1YJd2n3ksq/GRzzLNYJ6qlX2OW9zgvuE8WC47KjBWjf3kmQSLDQLoKxfpSXF6P/9uyI/rSivGw= 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=j06nIJDZ; 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="j06nIJDZ" 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 D4DDD201B; Thu, 28 May 2026 09:01:20 -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 B5F3B3F632; Thu, 28 May 2026 09:01:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1779984085; bh=Pb9KUS39SJ5eL/Q/btXVm8CCz+Epp99Lk53Gb8UqVPk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=j06nIJDZzjkPJV40mZ+R/MeV42TMdsDbkYLohE0T+wduF621CMnrKn+SPjjQ+sc5k StMzfr18i9Q/lv5o3ZtCANDP02cDsOJVOcOMgNhDRJYhjMW2IDa1DmINBz8yZCQLDM WQDTbVL9u8ACbFRk09KB4M+IIzqdgWXIeV6TGcnw= Date: Thu, 28 May 2026 17:01:22 +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@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> 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: <20260528152633.GH101133@e132581.arm.com> > 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? > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 0889937811cb..471824234800 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -882,16 +882,6 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > /* bit[12], Return stack enable bit */ > config->cfg |= TRCCONFIGR_RS; > > - /* > - * 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); > - } > - > /* branch broadcast - enable if selected and supported */ > if (ATTR_CFG_GET_FLD(attr, branch_broadcast)) { > if (!caps->trcbb) { > @@ -899,8 +889,6 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > * Missing BB support could cause silent decode errors > * so fail to open if it's not supported. > */ > - if (cfg_hash) > - cscfg_csdev_disable_active_config(csdev); > ret = -EINVAL; > goto out; > } else { > @@ -908,10 +896,31 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > } > } > > + /* > + * 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); > + } > + > out: > return ret; > } > > +static void etm4_clean_event_config(struct coresight_device *csdev, > + struct perf_event *event) > +{ > + struct perf_event_attr *attr = &event->attr; > + unsigned long cfg_hash; > + > + cfg_hash = ATTR_CFG_GET_FLD(attr, configid); > + if (cfg_hash) > + cscfg_csdev_disable_active_config(csdev); > +} > + > static int etm4_enable_perf(struct coresight_device *csdev, > struct perf_event *event, > struct coresight_path *path) > @@ -938,15 +947,14 @@ static int etm4_enable_perf(struct coresight_device *csdev, > > /* And enable it */ > ret = etm4_enable_hw(drvdata); > - if (ret) { > - if (ATTR_CFG_GET_FLD(attr, configid)) > - cscfg_csdev_disable_active_config(csdev); > - goto err; > - } > + if (ret) > + goto err_hw; > > csdev->path = path; > return 0; > > +err_hw: > + etm4_clean_event_config(csdev, event); > err: > /* Failed to start tracer; roll back to DISABLED mode */ > coresight_set_mode(csdev, CS_MODE_DISABLED); -- Sincerely, Yeoreum Yun