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

[Mathieu Poirier]

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 at 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 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 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?

> 
> 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