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

Mathieu Poirier
2020-08-18T17:40:43Z

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?

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