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

[Jie Gan]

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