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

[Tingwei Zhang <tingweiz@codeaurora.org>]

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@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@linaro.org>
> > Signed-off-by: Tingwei Zhang <tingwei@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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel