[PATCH v6 20/25] coresight: cti: don't disable ect device if it's not enabled

[Mike Leach]

Hi Tingwei,

On Tue, 4 Aug 2020 at 01:22, Tingwei Zhang <tingweiz at codeaurora.org> wrote:
>
> Hi Mike,
> On Tue, Aug 04, 2020 at 01:13:06AM +0800, Mike Leach wrote:
> > Hi Tingwei,
> >
> > On Fri, 31 Jul 2020 at 07:42, Tingwei Zhang <tingwei at codeaurora.org> wrote:
> > >
> > > If associated ect device is not enabled at first place, disable
> > > routine should not be called. Add ect_enabled flag to check whether
> > > ect device is enabled. Fix the issue in below case.  Ect device is
> > > not available when associated coresight device enabled and the
> > > association is established after coresight device is enabled.
> > >
> > > Signed-off-by: Mike Leach <mike.leach at linaro.org>
> > > Signed-off-by: Tingwei Zhang <tingwei at codeaurora.org>
> > > ---
> > >  drivers/hwtracing/coresight/coresight.c | 11 ++++++++---
> > >  include/linux/coresight.h               |  1 +
> > >  2 files changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > b/drivers/hwtracing/coresight/coresight.c
> > > index 8b55383cfcf1..83a46cf37968 100644
> > > --- a/drivers/hwtracing/coresight/coresight.c
> > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct
> > > coresight_device *csdev, bool enable)
> > >
> > >         if (!ect_csdev)
> > >                 return 0;
> > > +       if ((!ect_ops(ect_csdev)->enable) ||
> > > (!ect_ops(ect_csdev)->disable))
> > > +               return 0;
> > >
> > >         if (enable) {
> > > -               if (ect_ops(ect_csdev)->enable)
> > > -                       ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
> > > +               ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
> > > +               if (ect_ret)
> >
> > Should this be:-
> > if (!ect_ret)
> >
> > However, this problem is corrected in the next patch - so the set as
> > tested overall works.
>
> Thanks a lot for testing this series, Mike.
>
> Sorry for this stupid mistake. I'll address it in v7.
>
> >
> > This needs fixing or it might be better to combine this patch and the
> > next patch into a single patch?
> > Both are required because the CTI can be loaded as a module. Both
> > adjust the same block of functionality.
>
> I separate them into two since this patch adds ect_enabled flag and
> next patch add module and device reference. Kind of two things in
> my opinion. I'm open for discusion and merge them into one path.
>

I don't mind which solution is used.
I mention it because I have had objections in the past where two
patches in a set change the same piece of code.

Regards

Mike

> Thanks,
> Tingwei
>
> >
> > Mathieu may have a view on the best solution option here.
> >
> > Mike
> >
> >
> >
> > > +                       csdev->ect_enabled = true;
> > >         } else {
> > > -               if (ect_ops(ect_csdev)->disable)
> > > +               if (csdev->ect_enabled) {
> > >                         ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
> > > +                       csdev->ect_enabled = false;
> > > +               }
> > >         }
> > >
> > >         /* output warning if ECT enable is preventing trace operation */
> > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > > index 3bb738f9a326..7d3c87e5b97c 100644
> > > --- a/include/linux/coresight.h
> > > +++ b/include/linux/coresight.h
> > > @@ -208,6 +208,7 @@ struct coresight_device {
> > >         /* sysfs links between components */
> > >         int nr_links;
> > >         bool has_conns_grp;
> > > +       bool ect_enabled; /* true only if associated ect device is enabled
> > > */
> > >  };
> > >
> > >  /*
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > >
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK



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