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; 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);
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
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.
On Wed, Apr 02, 2025 at 02:50:22PM +0100, Mike Leach wrote:
[...]
@@ -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.
Okay, now I understand here avoids to disable source and links for a sink error.
then falls through to coresight_disable_path_from() - which the original code avoided and which also disables helpers a second time.
Seems to me, the conclusion for "disables helpers a second time" is incorrect.
I checked the coresight_disable_path_from() function, when the current 'nd' is passed to it, it will iterate from the _next_ node after 'nd'.
/* Here 'nd' will be skipped and start from the next node */ list_for_each_entry_continue(nd, &path->path_list, link) { ...
coresight_disable_helpers(csdev, path); }
This means the _current_ coresight device (here is sink device) will not disable its helpers. Could you confirm for this?
Thanks, Leo
-
Leo Yan -
Mike Leach