[PATCH v8 19/24] coresight: cti: don't disable ect device if it's not enabled
Mathieu Poirier
mathieu.poirier at linaro.org
Wed Aug 19 15:50:36 EDT 2020
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 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?
> >
>
> 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 at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list