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 C4F1118CC13 for ; Thu, 28 May 2026 14:43:44 +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=1779979426; cv=none; b=THmgjtFXS3E6wDZFDJwZkBK0OHk2PYBi60AyCgxHzdSvz4jf7eX+fSvj40BjCQa77F53lvFNvM/VA/BZhqjZK5TAa0rozozGG2/NbSGTEQ1aL2nINviZL+apKaUQN4uiL9HsNsCDQLr857MlgOo2fnOiFDMjlrIQCQnndvNO1LY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779979426; c=relaxed/simple; bh=TA4DO2tutucSmnsPezj88rJH8VgrWEtDABxFp+R8U54=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=VlOgRCIf1hzLUfyObidWUVYvyxWdcVHKSPLOVFjEWsSlH1AHNV038DZoEK/4whPFzjPXpJXtRg+ffa2XOjvii29kM+p3f7fadYpgiLgTt5uhN1jtCpffxL2In+rOTtaafsVOpHSbMYBOkdPD3oRsyVojIjqdRoYZtQRQuBSw3N0= 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=Qw/z2GHp; 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="Qw/z2GHp" 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 2547322BE; Thu, 28 May 2026 07:43:39 -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 C3F733F905; Thu, 28 May 2026 07:43:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1779979424; bh=TA4DO2tutucSmnsPezj88rJH8VgrWEtDABxFp+R8U54=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Qw/z2GHpbhVhhMELrrHLxE1NwtZBR9ALkpMzJrG0On1FMfpZ9QwfrXcaQOXxZVbfD JiDSZ6uWlv5HBtco0cbZM6z7VVbF/m/HxdFZH1rVxs5a8Jk74Z0kt0SND5NuqG4hsq q8T6xphiH9c8r+ll/KvnPvUkySjWhZR1qTs1aJM4= Date: Thu, 28 May 2026 15:43:40 +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> 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: <20260528143358.GF101133@e132581.arm.com> Hi Leo, > On Tue, May 19, 2026 at 04:48:08PM +0100, Yeoreum Yun wrote: > > In the perf enable path, there are missing cases where > > cscfg_csdev_disable_active_config() is not called: > > > > - Branch broadcast is selected but not supported by the hardware > > - etm4_enable_hw() fails > > > > This can lead to a leak of config_desc->active_cnt. > > Fix this by properly calling cscfg_csdev_disable_active_config() > > in these error paths. > > > > Fixes: 810ac401db1f ("coresight: etm4x: Add complex configuration handlers to etmv4") > > Signed-off-by: Yeoreum Yun > > --- > > .../hwtracing/coresight/coresight-etm4x-core.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > index f0a9f2ef4b27..0889937811cb 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > > @@ -899,6 +899,8 @@ 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 { > > @@ -915,6 +917,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, > > struct coresight_path *path) > > { > > struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > + struct perf_event_attr *attr = &event->attr; > > int ret; > > > > if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) > > @@ -926,7 +929,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, > > /* Configure the tracer based on the session's specifics */ > > ret = etm4_parse_event_config(csdev, event); > > if (ret) > > - goto out; > > + goto err; > > > > drvdata->trcid = path->trace_id; > > > > @@ -935,16 +938,19 @@ static int etm4_enable_perf(struct coresight_device *csdev, > > > > /* And enable it */ > > ret = etm4_enable_hw(drvdata); > > - > > -out: > > - /* Failed to start tracer; roll back to DISABLED mode */ > > if (ret) { > > - coresight_set_mode(csdev, CS_MODE_DISABLED); > > - return ret; > > + if (ATTR_CFG_GET_FLD(attr, configid)) > > + cscfg_csdev_disable_active_config(csdev); > > + goto err; > > } > > > > csdev->path = path; > > return 0; > > + > > +err: > > + /* Failed to start tracer; roll back to DISABLED mode */ > > + coresight_set_mode(csdev, CS_MODE_DISABLED); > > + return ret; > > } > > Seems to me, it is not readable if spread error handling into both child > and parent functions. I would like we can stick to use the same function > for resource allocation and rollback for failures. > > How about below change (I did not verify it yet): > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index 0889937811cb..8347efb2069b 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -794,8 +794,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > .ATTR_CFG_FLD_timestamp_CFG = U64_MAX, > }; > struct perf_event_attr *attr = &event->attr; > - unsigned long cfg_hash; > - int preset, cc_threshold; > + int cc_threshold; > u8 ts_level; > > /* Clear configuration from previous run */ > @@ -882,16 +881,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 +888,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 { > @@ -918,7 +905,8 @@ static int etm4_enable_perf(struct coresight_device *csdev, > { > struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > struct perf_event_attr *attr = &event->attr; > - int ret; > + unsigned long cfg_hash; > + int preset, ret; > > if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) > return -EINVAL; > @@ -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; > + } > + > drvdata->trcid = path->trace_id; > > /* Populate pause state */ > @@ -938,15 +938,15 @@ 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_enable_hw; > > csdev->path = path; > return 0; > > +err_enable_hw: > + if (cfg_hash) > + cscfg_csdev_disable_active_config(csdev); > err: > /* Failed to start tracer; roll back to DISABLED mode */ > coresight_set_mode(csdev, CS_MODE_DISABLED); No. since preset overrides the "perf configuratoin" formerly but this code makes it vice versa. 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. -- Sincerely, Yeoreum Yun