| Message ID | 20250402011832.2970072-4-yabinc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | coresight: catu: Introduce refcount and spinlock for enabling/disabling | expand |
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); > +err_helper: > coresight_disable_path_from(path, nd); > goto out; > } > -- > 2.49.0.472.ge94155a9ec-goog >
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
On 4/2/2025 10:12 PM, Leo Yan wrote: > 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? It seems there is an exception that the helper devices connected to the sink? The sink device may not the first the device to be enabled in the path if the sink device has a helper device. So I think we should add following logic before return? 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) { /* disable the helpers which connected to sink */ coresight_disable_helpers(csdev, path); goto out; } break; Thanks, Jie > > Thanks, > Leo >
On Wed, Apr 2, 2025 at 5:54 PM Jie Gan <quic_jiegan@quicinc.com> wrote: > > > > On 4/2/2025 10:12 PM, Leo Yan wrote: > > 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? Yes, without this patch: If the current coresight device fails to enable, the code calls coresight_disable_path_from() to disable devices and helpers after this device. But it doesn't disable helpers for the current device. With this patch: 1) When the helpers of the current device fails to enable, call coresight_disable_path_from() to disable following devices along the path. 2) When the current device fails to enable, first disable the helpers for the current devices, then call coresight_disable_path_from() to disable following devices and helpers. Regarding the comment about not calling coresight_disable_path_from() if a sink device fails to enable, I feel that's a case of it not being necessary, rather than something that causes logic error. So this patch calls it to avoid duplicating coresight_disable_helpers(). If my understanding is wrong, I am happy to change to duplicate coresight_disable_helpers() (in three places). > > It seems there is an exception that the helper devices connected to the > sink? The sink device may not the first the device to be enabled in the > path if the sink device has a helper device. > > So I think we should add following logic before return? > > 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) { > /* disable the helpers which connected to sink */ > coresight_disable_helpers(csdev, path); > goto out; > } > break; This is the change I initially considered. But there are two other cases (case CORESIGHT_DEV_TYPE_LINK and case default) that may need to call coresight_disable_helpers() before returning an error. > > > Thanks, > Jie > > > > > Thanks, > > Leo > > >
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); +err_helper: coresight_disable_path_from(path, nd); goto out; }
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(-)