Re: [PATCH v8 19/24] coresight: cti: don't disable ect device if it's not enabled

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Wed, Aug 19, 2020 at 09:55:33AM +0800, Tingwei Zhang wrote:
> On Wed, Aug 19, 2020 at 01:39:09AM +0800, Mathieu Poirier wrote:
> > On Mon, Aug 17, 2020 at 08:04:06PM +0100, Mike Leach wrote:
> > > Hi Mathieu,
> > > 
> > > On Mon, 17 Aug 2020 at 17:38, Mathieu Poirier
> > > <mathieu.poirier@linaro.org> wrote:
> > > >
> > > > On Fri, Aug 07, 2020 at 07:11:48PM +0800, Tingwei Zhang 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 d066411aa794..27ad8317e3cf 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)
> > > > > +                     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 */
> > > >
> > > > We have cti_config::enable_req_count and cti_config::hw_enabled, both
> > used in
> > > > cti_enable_hw() and cti_disable_hw().  I would have thought they'd be
> > sufficient
> > > > to address the counting problems.  If they are not I would much rather
> > see a
> > > > solution confined to the cti driver than in the core itself.
> > > >
> > > 
> > > This is related to the fact that under sysfs it is possible under
> > > sysfs to enable an etm e.g. etm1 without the cti module present, then
> > > insert the CTI module, then enable another ETM e.g etm2.
> > > This is an issue that is caused by the possibility of module load and
> > > unload, and though inadvisable from a system usage point of view -
> > > Tingwei correctly points out that it could happen.
> > > 
> > > At the point that the first ETM is enabled, the associated ect pointer
> > > would be NULL, and thus no attempt to enable the ect/CTI is made. The
> > > CTI module on load will set the ect pointers on all registered csdev
> > > devices, including ones that are already enabled - etm1.
> > > So when we come to disable etm1, it will try to disable a CTI that it
> > > did not enable - a fact that cannot be counted in the CTI driver as it
> > > was not there when the etm was enabled. So we have a flag in csdev to
> > > record if this csdev did in fact enable the associated device, so it
> > > is clear to disable it on shutdown.
> > 
> > In the above example the csdev structures associated with ETM1 and ETM2
> > will
> > have different csdev->ect_dev pointers, one for each CTI it is assocated
> > with.
> > And I would think the reference count for CTI1 would not have been
> > incremented
> > since it was never enabled.
> > 
> > What am I missing?
> > 
> 
> The issue here is we could have two or more coresght devices associated
> to single CTI device, especially for sys cti case. For example, we have
> coresight devcie A and B. They are assocaited to CTI device C. CTI module
> is not loaded at first. Device A is enabled. CTI C is not enabled. Then
> CTI module is loaded. Device B is enabled. CTI C is enabled with
> enable_req_count = 1. Device A is disabled. It decrease enable_req_count
> to 0, so CTI C is disabled. It's kinda weird to me that CTI C is enabled
> with device B and disabled by device A.

Thanks for taking the time to write this out, I now have a better understanding
of the problem.  On the flip side I hope to find a better solution, something I
will have to think about.  No need to wait on me though, resubmit a new revision
when ready and I'll see then.

> 
> The point is we need some flag in coresight device structure to save the
> status of associated CTI enablement when CTI module is not loaded yet.
> 
> Thanks,
> Tingwei
> 
> > > 
> > > Regards
> > > 
> > > Mike
> > > 
> > > 
> > > 
> > > > Thanks,
> > > > Mathieu
> > > >
> > > > >  };
> > > > >
> > > > >  /*
> > > > > --
> > > > > 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

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