Hi,
On Wed, 2 Apr 2025 at 14:32, Leo Yan leo.yan@arm.com wrote:
On Tue, Apr 01, 2025 at 06:22:46PM -0700, Yabin Cui wrote:
On Tue, Apr 1, 2025 at 6:19 PM Yabin Cui yabinc@google.com wrote:
When enabling a SINK or LINK type coresight device fails, the associated helpers should be disabled.
Signed-off-by: Yabin Cui yabinc@google.com Suggested-by: Suzuki K Poulose suzuki.poulose@arm.com
drivers/hwtracing/coresight/coresight-core.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index fb43ef6a3b1f..e3270d9b46c9 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, /* Enable all helpers adjacent to the path first */ ret = coresight_enable_helpers(csdev, mode, path); if (ret)
goto err;
goto err_helper; /* * ETF devices are tricky... They can be a link or a sink, * depending on how they are configured. If an ETF has been@@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, switch (type) { case CORESIGHT_DEV_TYPE_SINK: ret = coresight_enable_sink(csdev, mode, sink_data);
/** Sink is the first component turned on. If we* failed to enable the sink, there are no components* that need disabling. Disabling the path here* would mean we could disrupt an existing session.*/ if (ret)goto out;
goto err;
Going to err here is wrong. The comment above specifically states that we do _not_ want to disable the path, yet the new code flow disables helpers. then falls through to coresight_disable_path_from() - which the original code avoided and which also disables helpers a second time.
Please rework to get the correct program flow.
break; case CORESIGHT_DEV_TYPE_SOURCE: /* sources are enabled from either sysFS or Perf */@@ -507,6 +501,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode, out: return ret; err:
coresight_disable_helpers(csdev);
Drop this - put it where it is needed in the flow
Regards
Mike
Given it is unconditionally to enable helpers, I would suggest we disable helper unconditionally. Thus, we don't need to add a new 'err_helper' tag, simply reuse the 'err' tag would be fine.
I would like to know if mainatiners have different opinions.
Thanks, Leo
+err_helper: coresight_disable_path_from(path, nd); goto out; } -- 2.49.0.472.ge94155a9ec-goog
coresight_disable_path_from() also disables helpers.