[PATCH v3 2/2] coresight: core: Disable helpers for devices that fail to enable

[Yabin Cui]

On Fri, Apr 25, 2025 at 8:25 AM Mike Leach <mike.leach at linaro.org> wrote:
>
> On Tue, 15 Apr 2025 at 14:51, James Clark <james.clark at linaro.org> wrote:
> >
> >
> >
> > On 08/04/2025 8:59 pm, Yabin Cui wrote:
> > > When enabling a SINK or LINK type coresight device fails, the
> > > associated helpers should be disabled.
> > >
> > > Signed-off-by: Yabin Cui <yabinc at google.com>
> > > Suggested-by: Suzuki K Poulose <suzuki.poulose at arm.com>
> > > ---
> > >   drivers/hwtracing/coresight/coresight-core.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > > index fb43ef6a3b1f..a56ba9087538 100644
> > > --- a/drivers/hwtracing/coresight/coresight-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > > @@ -486,8 +486,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >                        * that need disabling. Disabling the path here
> > >                        * would mean we could disrupt an existing session.
> > >                        */
> > > -                     if (ret)
> > > +                     if (ret) {
> > > +                             coresight_disable_helpers(csdev);
> >
> > Hi Yabin,
> >
> > Unfortunately coresight_disable_helpers() takes a path pointer now so
> > this needs to be updated.
> >
> > I tested with that change made and it works ok.
> >
> > >                               goto out;
> > > +                     }
> > >                       break;
> > >               case CORESIGHT_DEV_TYPE_SOURCE:
> > >                       /* sources are enabled from either sysFS or Perf */
> > > @@ -496,10 +498,13 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> > >                       parent = list_prev_entry(nd, link)->csdev;
> > >                       child = list_next_entry(nd, link)->csdev;
> > >                       ret = coresight_enable_link(csdev, parent, child, source);
> > > -                     if (ret)
> > > +                     if (ret) {
> > > +                             coresight_disable_helpers(csdev);
> > >                               goto err;
> > > +                     }
> > >                       break;
> > >               default:
> > > +                     coresight_disable_helpers(csdev);
> >
> > Minor nit, you could collapse these last two into "goto
> > err_disable_helpers" and add another label before err: that disables
> > helpers before falling through to err:.
> >
> > Other than that:
> >
> > Reviewed-by: James Clark <james.clark at linaro.org>
> >
> > >                       goto err;
> > >               }
> > >       }
> >
>
> Subject to James' comments -
>
> Reviewed-by: Mike Leach <mike.leach at linaro.org>
>

Hi Mike, I have uploaded the v4 patch as suggested by James, in
"[PATCH v4 2/2] coresight: core: Disable helpers for devices that fail
to enable".
Could you help review the v4 patch? In that patch, Leo suggested consolidating
error handling. But you expressed concern about it when reviewing the v2 patch.


>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK