On 29/01/2025 1:02 pm, Jie Gan wrote:
On 1/29/2025 6:35 PM, James Clark wrote:
On 29/01/2025 12:46 am, Jie Gan wrote:
On 1/28/2025 7:55 PM, James Clark wrote:
On 24/01/2025 7:25 am, Jie Gan wrote:
The Coresight TMC Control Unit hosts miscellaneous configuration
registers
which control various features related to TMC ETR sink.
Based on the trace ID, which is programmed in the related CTCU ATID
register of a specific ETR, trace data with that trace ID gets into
the ETR buffer, while other trace data gets dropped.
Enabling source device sets one bit of the ATID register based on
source device's trace ID.
Disabling source device resets the bit according to the source
device's trace ID.
Signed-off-by: Jie Gan <quic_jiegan@xxxxxxxxxxx>
---
drivers/hwtracing/coresight/Kconfig | 12 +
drivers/hwtracing/coresight/Makefile | 1 +
drivers/hwtracing/coresight/coresight-ctcu.c | 276 ++++++++++++++
+ ++++
drivers/hwtracing/coresight/coresight-ctcu.h | 30 ++
include/linux/coresight.h | 3 +-
5 files changed, 321 insertions(+), 1 deletion(-)
create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>
[...]
+/*
+ * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
+ *
+ * Returns 0 indicates success. None-zero result means failure.
+ */
+static int ctcu_set_etr_traceid(struct coresight_device *csdev,
struct coresight_path *cs_path,
+ bool enable)
+{
+ struct coresight_device *sink = coresight_get_sink(cs_path-
>path);
+ struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev-
>dev.parent);
+ u8 trace_id = cs_path->trace_id;
+ int port_num;
+
+ if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) ||
IS_ERR_OR_NULL(drvdata)) {
+ dev_err(&csdev->dev, "Invalid parameters\n");
+ return -EINVAL;
+ }
+
+ port_num = ctcu_get_active_port(sink, csdev);
+ if (port_num < 0)
+ return -EINVAL;
+
+ /*
+ * Skip the disable session if more than one TPDM device that
+ * connected to the same TPDA device has been enabled.
+ */
+ if (enable)
+ atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
+ else {
+ if (atomic_dec_return(&drvdata->traceid_refcnt[port_num]
[trace_id]) > 0) {
+ dev_dbg(&csdev->dev, "Skip the disable session\n");
+ return 0;
+ }
+ }
+
+ dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
+
+ return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
Hi Jie,
Using atomic_dec_return() here doesn't prevent
__ctcu_set_etr_traceid() from running concurrent enable and
disables. Once you pass the atomic_dec_return() a second call to
enable it will mess it up.
I think you need a spinlock around the whole thing and then the
refcounts don't need to be atomics.
Hi, James
Thanks for comment. I may not fully tested my codes here. What I was
thinking is there's no way the refcnt could become a negative number
under current framework. So I just added spinlock in
__ctcu_set_etr_traceid() to ensure concurrent sessions correctly
manipulate the register.
As the trace_id related to the bit of the ATID register, I think the
concurrent processes are working fine with spinlock around read/write
register.
I may not fully got your point here. Please help me to correct it.
Thanks,
Jie
No it can't become negative, but the refcount can be a different state
to the one that was actually written:
CPU0 CPU1
---- ----
ctcu_set_etr_traceid(enable)
ctcu_set_etr_traceid(disable)
atomic_inc()
recount == 1
atomic_dec()
recount == 0
__ctcu_set_etr_traceid(disable)
Lock and write disable state to
device
__ctcu_set_etr_traceid(enable)
Lock and write enable state to
device
As you can see this leaves the device in an enabled state but the
refcount is 0.
Yes, you are right. I didnt consider this scenario. We definitely need
spinlock here.
This is also quite large if you use atomic types:
/* refcnt for each traceid of each sink */
atomic_t traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
Presumably you can't have the refcount for each ID be higher than the
max number of TPDMs connected? If you make the locked area a bit wider
you don't need atomic types and also solve the above problem. So you
could do u8, or DECLARE_BITMAP() and bitmap_read() etc to read 3 bit
values. Or however wide it needs to be.
The original purpose of using atomic here is trying to narrow the locked
area.
I think u8 is ok here.
u8 traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP] will cost
224 bytes, I think it's acceptable here.
Thanks,
Jie
Yep u8 sounds ok then
