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

all subsystems

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
[off-list ref] 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 <redacted>
Signed-off-by: Tingwei Zhang <redacted>
---
 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.

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help