[PATCH v2 3/3] coresight: core: Disable helpers for devices that fail to enable

[Yabin Cui]

On Wed, Apr 2, 2025 at 5:54 PM Jie Gan <quic_jiegan at 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
> >
>