* [PATCH v3] coresight: tpdm: add traceid_show for checking traceid @ 2026-03-25 3:10 Jie Gan 2026-03-30 14:55 ` James Clark 0 siblings, 1 reply; 9+ messages in thread From: Jie Gan @ 2026-03-25 3:10 UTC (permalink / raw) To: Suzuki K Poulose, Mike Leach, James Clark, Leo Yan, Alexander Shishkin, Tingwei Zhang Cc: coresight, linux-arm-kernel, linux-kernel, Jie Gan Save the trace ID in drvdata during TPDM enablement and expose it to userspace to support trace data parsing. The TPDM device’s trace ID corresponds to the trace ID allocated to the connected TPDA device. Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> --- Changes in v3: 1. Only allow user to read the traceid while the TPDM device is enabled. - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show-for-tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com Changes in V2: 1. Use sysfs_emit instead of sprintf. Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show-for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ --- drivers/hwtracing/coresight/coresight-tpdm.c | 34 +++++++++++++++++++++++++++- drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index da77bdaad0a4..c8339b973bfc 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata) static int tpdm_enable(struct coresight_device *csdev, struct perf_event *event, enum cs_mode mode, - __maybe_unused struct coresight_path *path) + struct coresight_path *path) { struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device *csdev, struct perf_event *event, } __tpdm_enable(drvdata); + drvdata->traceid = path->trace_id; drvdata->enable = true; spin_unlock(&drvdata->spinlock); @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { .attrs = tpdm_attrs, }; +static ssize_t traceid_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + unsigned long val; + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); + + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) + return -EINVAL; + + val = drvdata->traceid; + return sysfs_emit(buf, "%#lx\n", val); +} +static DEVICE_ATTR_RO(traceid); + +static struct attribute *traceid_attrs[] = { + &dev_attr_traceid.attr, + NULL, +}; + +static struct attribute_group traceid_attr_grp = { + .attrs = traceid_attrs, +}; + static ssize_t dsb_mode_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -1367,6 +1391,12 @@ static const struct attribute_group *tpdm_attr_grps[] = { &tpdm_cmb_patt_grp, &tpdm_cmb_msr_grp, &tpdm_mcmb_attr_grp, + &traceid_attr_grp, + NULL, +}; + +static const struct attribute_group *static_tpdm_attr_grps[] = { + &traceid_attr_grp, NULL, }; @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, struct resource *res) desc.access = CSDEV_ACCESS_IOMEM(base); if (res) desc.groups = tpdm_attr_grps; + else + desc.groups = static_tpdm_attr_grps; drvdata->csdev = coresight_register(&desc); if (IS_ERR(drvdata->csdev)) return PTR_ERR(drvdata->csdev); diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h index 2867f3ab8186..11da64e1ade8 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.h +++ b/drivers/hwtracing/coresight/coresight-tpdm.h @@ -300,6 +300,7 @@ struct cmb_dataset { * @cmb Specifics associated to TPDM CMB. * @dsb_msr_num Number of MSR supported by DSB TPDM * @cmb_msr_num Number of MSR supported by CMB TPDM + * @traceid Trace ID of the path. */ struct tpdm_drvdata { @@ -313,6 +314,7 @@ struct tpdm_drvdata { struct cmb_dataset *cmb; u32 dsb_msr_num; u32 cmb_msr_num; + u8 traceid; }; /* Enumerate members of various datasets */ --- base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 Best regards, -- Jie Gan <jie.gan@oss.qualcomm.com> ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-25 3:10 [PATCH v3] coresight: tpdm: add traceid_show for checking traceid Jie Gan @ 2026-03-30 14:55 ` James Clark 2026-03-31 1:29 ` Jie Gan 0 siblings, 1 reply; 9+ messages in thread From: James Clark @ 2026-03-30 14:55 UTC (permalink / raw) To: Jie Gan, Suzuki K Poulose Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang On 25/03/2026 3:10 am, Jie Gan wrote: > Save the trace ID in drvdata during TPDM enablement and expose it > to userspace to support trace data parsing. > > The TPDM device’s trace ID corresponds to the trace ID allocated > to the connected TPDA device. > > Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> > --- > Changes in v3: > 1. Only allow user to read the traceid while the TPDM device is enabled. > - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show-for-tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com > > Changes in V2: > 1. Use sysfs_emit instead of sprintf. > Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show-for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 34 +++++++++++++++++++++++++++- > drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index da77bdaad0a4..c8339b973bfc 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata *drvdata) > > static int tpdm_enable(struct coresight_device *csdev, struct perf_event *event, > enum cs_mode mode, > - __maybe_unused struct coresight_path *path) > + struct coresight_path *path) > { > struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > > @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device *csdev, struct perf_event *event, > } > > __tpdm_enable(drvdata); > + drvdata->traceid = path->trace_id; > drvdata->enable = true; > spin_unlock(&drvdata->spinlock); > > @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { > .attrs = tpdm_attrs, > }; > > +static ssize_t traceid_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + unsigned long val; > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) > + return -EINVAL; > + > + val = drvdata->traceid; You probably need to take the coresight_mutex here otherwise you could still return an invalid or stale value despite checking the mode. There might also be some value in it returning the last used trace ID even if the mode isn't enabled anymore. Because you can still read out of the sink after disabling, so it makes more sense for a script to read it at that point rather than when it's enabled. Also, you probably don't want to be doing other things in your script in the point between enabling and disabling. > + return sysfs_emit(buf, "%#lx\n", val); > +} > +static DEVICE_ATTR_RO(traceid); > + > +static struct attribute *traceid_attrs[] = { > + &dev_attr_traceid.attr, > + NULL, > +}; > + > +static struct attribute_group traceid_attr_grp = { > + .attrs = traceid_attrs, > +}; > + > static ssize_t dsb_mode_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -1367,6 +1391,12 @@ static const struct attribute_group *tpdm_attr_grps[] = { > &tpdm_cmb_patt_grp, > &tpdm_cmb_msr_grp, > &tpdm_mcmb_attr_grp, > + &traceid_attr_grp, > + NULL, > +}; > + > +static const struct attribute_group *static_tpdm_attr_grps[] = { > + &traceid_attr_grp, > NULL, > }; > > @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, struct resource *res) > desc.access = CSDEV_ACCESS_IOMEM(base); > if (res) > desc.groups = tpdm_attr_grps; > + else > + desc.groups = static_tpdm_attr_grps; > drvdata->csdev = coresight_register(&desc); > if (IS_ERR(drvdata->csdev)) > return PTR_ERR(drvdata->csdev); > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > index 2867f3ab8186..11da64e1ade8 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.h > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -300,6 +300,7 @@ struct cmb_dataset { > * @cmb Specifics associated to TPDM CMB. > * @dsb_msr_num Number of MSR supported by DSB TPDM > * @cmb_msr_num Number of MSR supported by CMB TPDM > + * @traceid Trace ID of the path. > */ > > struct tpdm_drvdata { > @@ -313,6 +314,7 @@ struct tpdm_drvdata { > struct cmb_dataset *cmb; > u32 dsb_msr_num; > u32 cmb_msr_num; > + u8 traceid; > }; > > /* Enumerate members of various datasets */ > > --- > base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 > change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 > > Best regards, ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-30 14:55 ` James Clark @ 2026-03-31 1:29 ` Jie Gan 2026-03-31 3:18 ` Jie Gan 0 siblings, 1 reply; 9+ messages in thread From: Jie Gan @ 2026-03-31 1:29 UTC (permalink / raw) To: James Clark, Suzuki K Poulose Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang Hi James, On 3/30/2026 10:55 PM, James Clark wrote: > > > On 25/03/2026 3:10 am, Jie Gan wrote: >> Save the trace ID in drvdata during TPDM enablement and expose it >> to userspace to support trace data parsing. >> >> The TPDM device’s trace ID corresponds to the trace ID allocated >> to the connected TPDA device. >> >> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >> --- >> Changes in v3: >> 1. Only allow user to read the traceid while the TPDM device is enabled. >> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show-for- >> tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >> >> Changes in V2: >> 1. Use sysfs_emit instead of sprintf. >> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- >> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >> --- >> drivers/hwtracing/coresight/coresight-tpdm.c | 34 ++++++++++++++++++ >> +++++++++- >> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >> 2 files changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/ >> hwtracing/coresight/coresight-tpdm.c >> index da77bdaad0a4..c8339b973bfc 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >> *drvdata) >> static int tpdm_enable(struct coresight_device *csdev, struct >> perf_event *event, >> enum cs_mode mode, >> - __maybe_unused struct coresight_path *path) >> + struct coresight_path *path) >> { >> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >> *csdev, struct perf_event *event, >> } >> __tpdm_enable(drvdata); >> + drvdata->traceid = path->trace_id; >> drvdata->enable = true; >> spin_unlock(&drvdata->spinlock); >> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >> .attrs = tpdm_attrs, >> }; >> +static ssize_t traceid_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + unsigned long val; >> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >> + >> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >> + return -EINVAL; >> + >> + val = drvdata->traceid; > > You probably need to take the coresight_mutex here otherwise you could > still return an invalid or stale value despite checking the mode. > Acked. I have missed this potential race condition. > There might also be some value in it returning the last used trace ID > even if the mode isn't enabled anymore. Because you can still read out > of the sink after disabling, so it makes more sense for a script to read > it at that point rather than when it's enabled. Also, you probably don't > want to be doing other things in your script in the point between > enabling and disabling. That's making sense. I shouldnt add such restriction for the read process. Scenarios for reading: 1. device is enabled -> trace ID is valid 2. device is enabled then disabled -> trace ID is valid for the last trace event 3. device is never enabled -> invalid trace ID (value 0) we only need to check the validation of the trace ID. mutex_lock(&coresight_mutex); val = drvdata->traceid; mutex_unlock(&coresight_mutex); if (!val) return -EINVAL; return sysfs_emit(buf, "%#lx\n", val); Thanks, Jie > >> + return sysfs_emit(buf, "%#lx\n", val); >> +} >> +static DEVICE_ATTR_RO(traceid); >> + >> +static struct attribute *traceid_attrs[] = { >> + &dev_attr_traceid.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group traceid_attr_grp = { >> + .attrs = traceid_attrs, >> +}; >> + >> static ssize_t dsb_mode_show(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> @@ -1367,6 +1391,12 @@ static const struct attribute_group >> *tpdm_attr_grps[] = { >> &tpdm_cmb_patt_grp, >> &tpdm_cmb_msr_grp, >> &tpdm_mcmb_attr_grp, >> + &traceid_attr_grp, >> + NULL, >> +}; >> + >> +static const struct attribute_group *static_tpdm_attr_grps[] = { >> + &traceid_attr_grp, >> NULL, >> }; >> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, struct >> resource *res) >> desc.access = CSDEV_ACCESS_IOMEM(base); >> if (res) >> desc.groups = tpdm_attr_grps; >> + else >> + desc.groups = static_tpdm_attr_grps; >> drvdata->csdev = coresight_register(&desc); >> if (IS_ERR(drvdata->csdev)) >> return PTR_ERR(drvdata->csdev); >> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/ >> hwtracing/coresight/coresight-tpdm.h >> index 2867f3ab8186..11da64e1ade8 100644 >> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >> @@ -300,6 +300,7 @@ struct cmb_dataset { >> * @cmb Specifics associated to TPDM CMB. >> * @dsb_msr_num Number of MSR supported by DSB TPDM >> * @cmb_msr_num Number of MSR supported by CMB TPDM >> + * @traceid Trace ID of the path. >> */ >> struct tpdm_drvdata { >> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >> struct cmb_dataset *cmb; >> u32 dsb_msr_num; >> u32 cmb_msr_num; >> + u8 traceid; >> }; >> /* Enumerate members of various datasets */ >> >> --- >> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >> >> Best regards, > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-31 1:29 ` Jie Gan @ 2026-03-31 3:18 ` Jie Gan 2026-03-31 9:26 ` James Clark 0 siblings, 1 reply; 9+ messages in thread From: Jie Gan @ 2026-03-31 3:18 UTC (permalink / raw) To: James Clark, Suzuki K Poulose Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang Hi James, On 3/31/2026 9:29 AM, Jie Gan wrote: > > Hi James, > > On 3/30/2026 10:55 PM, James Clark wrote: >> >> >> On 25/03/2026 3:10 am, Jie Gan wrote: >>> Save the trace ID in drvdata during TPDM enablement and expose it >>> to userspace to support trace data parsing. >>> >>> The TPDM device’s trace ID corresponds to the trace ID allocated >>> to the connected TPDA device. >>> >>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>> --- >>> Changes in v3: >>> 1. Only allow user to read the traceid while the TPDM device is enabled. >>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>> >>> Changes in V2: >>> 1. Use sysfs_emit instead of sprintf. >>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- >>> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>> --- >>> drivers/hwtracing/coresight/coresight-tpdm.c | 34 +++++++++++++++++ >>> + +++++++++- >>> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >>> 2 files changed, 35 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/ >>> hwtracing/coresight/coresight-tpdm.c >>> index da77bdaad0a4..c8339b973bfc 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>> *drvdata) >>> static int tpdm_enable(struct coresight_device *csdev, struct >>> perf_event *event, >>> enum cs_mode mode, >>> - __maybe_unused struct coresight_path *path) >>> + struct coresight_path *path) >>> { >>> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>> *csdev, struct perf_event *event, >>> } >>> __tpdm_enable(drvdata); >>> + drvdata->traceid = path->trace_id; >>> drvdata->enable = true; >>> spin_unlock(&drvdata->spinlock); >>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>> .attrs = tpdm_attrs, >>> }; >>> +static ssize_t traceid_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + unsigned long val; >>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>> + >>> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>> + return -EINVAL; >>> + >>> + val = drvdata->traceid; >> >> You probably need to take the coresight_mutex here otherwise you could >> still return an invalid or stale value despite checking the mode. >> > > Acked. I have missed this potential race condition. > >> There might also be some value in it returning the last used trace ID >> even if the mode isn't enabled anymore. Because you can still read out >> of the sink after disabling, so it makes more sense for a script to >> read it at that point rather than when it's enabled. Also, you >> probably don't want to be doing other things in your script in the >> point between enabling and disabling. > > That's making sense. I shouldnt add such restriction for the read process. > I missed one point in last message. Is that acceptable to export the coresight_mutex from the core module? Currently, the coresight_mutex is used within the module only. Thanks, Jie > Scenarios for reading: > 1. device is enabled -> trace ID is valid > 2. device is enabled then disabled -> trace ID is valid for the last > trace event > 3. device is never enabled -> invalid trace ID (value 0) > > we only need to check the validation of the trace ID. > > mutex_lock(&coresight_mutex); > val = drvdata->traceid; > mutex_unlock(&coresight_mutex); > > if (!val) > return -EINVAL; > > return sysfs_emit(buf, "%#lx\n", val); > > Thanks, > Jie > >> >>> + return sysfs_emit(buf, "%#lx\n", val); >>> +} >>> +static DEVICE_ATTR_RO(traceid); >>> + >>> +static struct attribute *traceid_attrs[] = { >>> + &dev_attr_traceid.attr, >>> + NULL, >>> +}; >>> + >>> +static struct attribute_group traceid_attr_grp = { >>> + .attrs = traceid_attrs, >>> +}; >>> + >>> static ssize_t dsb_mode_show(struct device *dev, >>> struct device_attribute *attr, >>> char *buf) >>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>> *tpdm_attr_grps[] = { >>> &tpdm_cmb_patt_grp, >>> &tpdm_cmb_msr_grp, >>> &tpdm_mcmb_attr_grp, >>> + &traceid_attr_grp, >>> + NULL, >>> +}; >>> + >>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>> + &traceid_attr_grp, >>> NULL, >>> }; >>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>> struct resource *res) >>> desc.access = CSDEV_ACCESS_IOMEM(base); >>> if (res) >>> desc.groups = tpdm_attr_grps; >>> + else >>> + desc.groups = static_tpdm_attr_grps; >>> drvdata->csdev = coresight_register(&desc); >>> if (IS_ERR(drvdata->csdev)) >>> return PTR_ERR(drvdata->csdev); >>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/ >>> hwtracing/coresight/coresight-tpdm.h >>> index 2867f3ab8186..11da64e1ade8 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>> * @cmb Specifics associated to TPDM CMB. >>> * @dsb_msr_num Number of MSR supported by DSB TPDM >>> * @cmb_msr_num Number of MSR supported by CMB TPDM >>> + * @traceid Trace ID of the path. >>> */ >>> struct tpdm_drvdata { >>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>> struct cmb_dataset *cmb; >>> u32 dsb_msr_num; >>> u32 cmb_msr_num; >>> + u8 traceid; >>> }; >>> /* Enumerate members of various datasets */ >>> >>> --- >>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>> >>> Best regards, >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-31 3:18 ` Jie Gan @ 2026-03-31 9:26 ` James Clark 2026-03-31 9:33 ` Jie Gan 2026-03-31 9:42 ` Suzuki K Poulose 0 siblings, 2 replies; 9+ messages in thread From: James Clark @ 2026-03-31 9:26 UTC (permalink / raw) To: Jie Gan, Suzuki K Poulose Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang On 31/03/2026 4:18 am, Jie Gan wrote: > Hi James, > > On 3/31/2026 9:29 AM, Jie Gan wrote: >> >> Hi James, >> >> On 3/30/2026 10:55 PM, James Clark wrote: >>> >>> >>> On 25/03/2026 3:10 am, Jie Gan wrote: >>>> Save the trace ID in drvdata during TPDM enablement and expose it >>>> to userspace to support trace data parsing. >>>> >>>> The TPDM device’s trace ID corresponds to the trace ID allocated >>>> to the connected TPDA device. >>>> >>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>>> --- >>>> Changes in v3: >>>> 1. Only allow user to read the traceid while the TPDM device is >>>> enabled. >>>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>>> >>>> Changes in V2: >>>> 1. Use sysfs_emit instead of sprintf. >>>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- >>>> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>>> --- >>>> drivers/hwtracing/coresight/coresight-tpdm.c | 34 ++++++++++++++++ >>>> + + +++++++++- >>>> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/ >>>> hwtracing/coresight/coresight-tpdm.c >>>> index da77bdaad0a4..c8339b973bfc 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>>> *drvdata) >>>> static int tpdm_enable(struct coresight_device *csdev, struct >>>> perf_event *event, >>>> enum cs_mode mode, >>>> - __maybe_unused struct coresight_path *path) >>>> + struct coresight_path *path) >>>> { >>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev- >>>> >dev.parent); >>>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>>> *csdev, struct perf_event *event, >>>> } >>>> __tpdm_enable(drvdata); >>>> + drvdata->traceid = path->trace_id; >>>> drvdata->enable = true; >>>> spin_unlock(&drvdata->spinlock); >>>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>>> .attrs = tpdm_attrs, >>>> }; >>>> +static ssize_t traceid_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + unsigned long val; >>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>> + >>>> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>>> + return -EINVAL; >>>> + >>>> + val = drvdata->traceid; >>> >>> You probably need to take the coresight_mutex here otherwise you >>> could still return an invalid or stale value despite checking the mode. >>> >> >> Acked. I have missed this potential race condition. >> >>> There might also be some value in it returning the last used trace ID >>> even if the mode isn't enabled anymore. Because you can still read >>> out of the sink after disabling, so it makes more sense for a script >>> to read it at that point rather than when it's enabled. Also, you >>> probably don't want to be doing other things in your script in the >>> point between enabling and disabling. >> >> That's making sense. I shouldnt add such restriction for the read >> process. >> > > I missed one point in last message. > > Is that acceptable to export the coresight_mutex from the core module? > Currently, the coresight_mutex is used within the module only. > > Thanks, > Jie > If the plan is to only check for non-zero trace ID and not check the mode any more then you don't need to lock. Maybe lets see what Suzuki thinks about returning the last trace ID though as it was his idea to add -EINVAL. >> Scenarios for reading: >> 1. device is enabled -> trace ID is valid >> 2. device is enabled then disabled -> trace ID is valid for the last >> trace event >> 3. device is never enabled -> invalid trace ID (value 0) >> >> we only need to check the validation of the trace ID. >> >> mutex_lock(&coresight_mutex); >> val = drvdata->traceid; >> mutex_unlock(&coresight_mutex); >> >> if (!val) >> return -EINVAL; >> >> return sysfs_emit(buf, "%#lx\n", val); >> >> Thanks, >> Jie >> >>> >>>> + return sysfs_emit(buf, "%#lx\n", val); >>>> +} >>>> +static DEVICE_ATTR_RO(traceid); >>>> + >>>> +static struct attribute *traceid_attrs[] = { >>>> + &dev_attr_traceid.attr, >>>> + NULL, >>>> +}; >>>> + >>>> +static struct attribute_group traceid_attr_grp = { >>>> + .attrs = traceid_attrs, >>>> +}; >>>> + >>>> static ssize_t dsb_mode_show(struct device *dev, >>>> struct device_attribute *attr, >>>> char *buf) >>>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>>> *tpdm_attr_grps[] = { >>>> &tpdm_cmb_patt_grp, >>>> &tpdm_cmb_msr_grp, >>>> &tpdm_mcmb_attr_grp, >>>> + &traceid_attr_grp, >>>> + NULL, >>>> +}; >>>> + >>>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>>> + &traceid_attr_grp, >>>> NULL, >>>> }; >>>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>>> struct resource *res) >>>> desc.access = CSDEV_ACCESS_IOMEM(base); >>>> if (res) >>>> desc.groups = tpdm_attr_grps; >>>> + else >>>> + desc.groups = static_tpdm_attr_grps; >>>> drvdata->csdev = coresight_register(&desc); >>>> if (IS_ERR(drvdata->csdev)) >>>> return PTR_ERR(drvdata->csdev); >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/ >>>> hwtracing/coresight/coresight-tpdm.h >>>> index 2867f3ab8186..11da64e1ade8 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>>> * @cmb Specifics associated to TPDM CMB. >>>> * @dsb_msr_num Number of MSR supported by DSB TPDM >>>> * @cmb_msr_num Number of MSR supported by CMB TPDM >>>> + * @traceid Trace ID of the path. >>>> */ >>>> struct tpdm_drvdata { >>>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>>> struct cmb_dataset *cmb; >>>> u32 dsb_msr_num; >>>> u32 cmb_msr_num; >>>> + u8 traceid; >>>> }; >>>> /* Enumerate members of various datasets */ >>>> >>>> --- >>>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>>> >>>> Best regards, >>> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-31 9:26 ` James Clark @ 2026-03-31 9:33 ` Jie Gan 2026-03-31 9:44 ` James Clark 2026-03-31 9:42 ` Suzuki K Poulose 1 sibling, 1 reply; 9+ messages in thread From: Jie Gan @ 2026-03-31 9:33 UTC (permalink / raw) To: James Clark, Suzuki K Poulose Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang On 3/31/2026 5:26 PM, James Clark wrote: > > > On 31/03/2026 4:18 am, Jie Gan wrote: >> Hi James, >> >> On 3/31/2026 9:29 AM, Jie Gan wrote: >>> >>> Hi James, >>> >>> On 3/30/2026 10:55 PM, James Clark wrote: >>>> >>>> >>>> On 25/03/2026 3:10 am, Jie Gan wrote: >>>>> Save the trace ID in drvdata during TPDM enablement and expose it >>>>> to userspace to support trace data parsing. >>>>> >>>>> The TPDM device’s trace ID corresponds to the trace ID allocated >>>>> to the connected TPDA device. >>>>> >>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>>>> --- >>>>> Changes in v3: >>>>> 1. Only allow user to read the traceid while the TPDM device is >>>>> enabled. >>>>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>>>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>>>> >>>>> Changes in V2: >>>>> 1. Use sysfs_emit instead of sprintf. >>>>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- >>>>> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>>>> --- >>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 34 +++++++++++++++ >>>>> + + + +++++++++- >>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >>>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/ >>>>> drivers/ hwtracing/coresight/coresight-tpdm.c >>>>> index da77bdaad0a4..c8339b973bfc 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>>>> *drvdata) >>>>> static int tpdm_enable(struct coresight_device *csdev, struct >>>>> perf_event *event, >>>>> enum cs_mode mode, >>>>> - __maybe_unused struct coresight_path *path) >>>>> + struct coresight_path *path) >>>>> { >>>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev- >>>>> >dev.parent); >>>>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>>>> *csdev, struct perf_event *event, >>>>> } >>>>> __tpdm_enable(drvdata); >>>>> + drvdata->traceid = path->trace_id; >>>>> drvdata->enable = true; >>>>> spin_unlock(&drvdata->spinlock); >>>>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>>>> .attrs = tpdm_attrs, >>>>> }; >>>>> +static ssize_t traceid_show(struct device *dev, >>>>> + struct device_attribute *attr, char *buf) >>>>> +{ >>>>> + unsigned long val; >>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>>> + >>>>> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>>>> + return -EINVAL; >>>>> + >>>>> + val = drvdata->traceid; >>>> >>>> You probably need to take the coresight_mutex here otherwise you >>>> could still return an invalid or stale value despite checking the mode. >>>> >>> >>> Acked. I have missed this potential race condition. >>> >>>> There might also be some value in it returning the last used trace >>>> ID even if the mode isn't enabled anymore. Because you can still >>>> read out of the sink after disabling, so it makes more sense for a >>>> script to read it at that point rather than when it's enabled. Also, >>>> you probably don't want to be doing other things in your script in >>>> the point between enabling and disabling. >>> >>> That's making sense. I shouldnt add such restriction for the read >>> process. >>> >> >> I missed one point in last message. >> >> Is that acceptable to export the coresight_mutex from the core module? >> Currently, the coresight_mutex is used within the module only. >> >> Thanks, >> Jie >> > > If the plan is to only check for non-zero trace ID and not check the > mode any more then you don't need to lock. Maybe lets see what Suzuki > thinks about returning the last trace ID though as it was his idea to > add -EINVAL. > Sure. The trace ID is allocated during TPDA device probe and remains unchanged for the entire lifetime of the device. Thanks, Jie >>> Scenarios for reading: >>> 1. device is enabled -> trace ID is valid >>> 2. device is enabled then disabled -> trace ID is valid for the last >>> trace event >>> 3. device is never enabled -> invalid trace ID (value 0) >>> >>> we only need to check the validation of the trace ID. >>> >>> mutex_lock(&coresight_mutex); >>> val = drvdata->traceid; >>> mutex_unlock(&coresight_mutex); >>> >>> if (!val) >>> return -EINVAL; >>> >>> return sysfs_emit(buf, "%#lx\n", val); >>> >>> Thanks, >>> Jie >>> >>>> >>>>> + return sysfs_emit(buf, "%#lx\n", val); >>>>> +} >>>>> +static DEVICE_ATTR_RO(traceid); >>>>> + >>>>> +static struct attribute *traceid_attrs[] = { >>>>> + &dev_attr_traceid.attr, >>>>> + NULL, >>>>> +}; >>>>> + >>>>> +static struct attribute_group traceid_attr_grp = { >>>>> + .attrs = traceid_attrs, >>>>> +}; >>>>> + >>>>> static ssize_t dsb_mode_show(struct device *dev, >>>>> struct device_attribute *attr, >>>>> char *buf) >>>>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>>>> *tpdm_attr_grps[] = { >>>>> &tpdm_cmb_patt_grp, >>>>> &tpdm_cmb_msr_grp, >>>>> &tpdm_mcmb_attr_grp, >>>>> + &traceid_attr_grp, >>>>> + NULL, >>>>> +}; >>>>> + >>>>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>>>> + &traceid_attr_grp, >>>>> NULL, >>>>> }; >>>>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>>>> struct resource *res) >>>>> desc.access = CSDEV_ACCESS_IOMEM(base); >>>>> if (res) >>>>> desc.groups = tpdm_attr_grps; >>>>> + else >>>>> + desc.groups = static_tpdm_attr_grps; >>>>> drvdata->csdev = coresight_register(&desc); >>>>> if (IS_ERR(drvdata->csdev)) >>>>> return PTR_ERR(drvdata->csdev); >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/ >>>>> drivers/ hwtracing/coresight/coresight-tpdm.h >>>>> index 2867f3ab8186..11da64e1ade8 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>>>> * @cmb Specifics associated to TPDM CMB. >>>>> * @dsb_msr_num Number of MSR supported by DSB TPDM >>>>> * @cmb_msr_num Number of MSR supported by CMB TPDM >>>>> + * @traceid Trace ID of the path. >>>>> */ >>>>> struct tpdm_drvdata { >>>>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>>>> struct cmb_dataset *cmb; >>>>> u32 dsb_msr_num; >>>>> u32 cmb_msr_num; >>>>> + u8 traceid; >>>>> }; >>>>> /* Enumerate members of various datasets */ >>>>> >>>>> --- >>>>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>>>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>>>> >>>>> Best regards, >>>> >>> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-31 9:33 ` Jie Gan @ 2026-03-31 9:44 ` James Clark 0 siblings, 0 replies; 9+ messages in thread From: James Clark @ 2026-03-31 9:44 UTC (permalink / raw) To: Jie Gan, Suzuki K Poulose Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang On 31/03/2026 10:33 am, Jie Gan wrote: > > > On 3/31/2026 5:26 PM, James Clark wrote: >> >> >> On 31/03/2026 4:18 am, Jie Gan wrote: >>> Hi James, >>> >>> On 3/31/2026 9:29 AM, Jie Gan wrote: >>>> >>>> Hi James, >>>> >>>> On 3/30/2026 10:55 PM, James Clark wrote: >>>>> >>>>> >>>>> On 25/03/2026 3:10 am, Jie Gan wrote: >>>>>> Save the trace ID in drvdata during TPDM enablement and expose it >>>>>> to userspace to support trace data parsing. >>>>>> >>>>>> The TPDM device’s trace ID corresponds to the trace ID allocated >>>>>> to the connected TPDA device. >>>>>> >>>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>>>>> --- >>>>>> Changes in v3: >>>>>> 1. Only allow user to read the traceid while the TPDM device is >>>>>> enabled. >>>>>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>>>>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>>>>> >>>>>> Changes in V2: >>>>>> 1. Use sysfs_emit instead of sprintf. >>>>>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid- >>>>>> show- for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>>>>> --- >>>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 34 ++++++++++++++ >>>>>> + + + + +++++++++- >>>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >>>>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/ >>>>>> drivers/ hwtracing/coresight/coresight-tpdm.c >>>>>> index da77bdaad0a4..c8339b973bfc 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>>>>> *drvdata) >>>>>> static int tpdm_enable(struct coresight_device *csdev, struct >>>>>> perf_event *event, >>>>>> enum cs_mode mode, >>>>>> - __maybe_unused struct coresight_path *path) >>>>>> + struct coresight_path *path) >>>>>> { >>>>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev- >>>>>> >dev.parent); >>>>>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>>>>> *csdev, struct perf_event *event, >>>>>> } >>>>>> __tpdm_enable(drvdata); >>>>>> + drvdata->traceid = path->trace_id; >>>>>> drvdata->enable = true; >>>>>> spin_unlock(&drvdata->spinlock); >>>>>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>>>>> .attrs = tpdm_attrs, >>>>>> }; >>>>>> +static ssize_t traceid_show(struct device *dev, >>>>>> + struct device_attribute *attr, char *buf) >>>>>> +{ >>>>>> + unsigned long val; >>>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>>>> + >>>>>> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + val = drvdata->traceid; >>>>> >>>>> You probably need to take the coresight_mutex here otherwise you >>>>> could still return an invalid or stale value despite checking the >>>>> mode. >>>>> >>>> >>>> Acked. I have missed this potential race condition. >>>> >>>>> There might also be some value in it returning the last used trace >>>>> ID even if the mode isn't enabled anymore. Because you can still >>>>> read out of the sink after disabling, so it makes more sense for a >>>>> script to read it at that point rather than when it's enabled. >>>>> Also, you probably don't want to be doing other things in your >>>>> script in the point between enabling and disabling. >>>> >>>> That's making sense. I shouldnt add such restriction for the read >>>> process. >>>> >>> >>> I missed one point in last message. >>> >>> Is that acceptable to export the coresight_mutex from the core module? >>> Currently, the coresight_mutex is used within the module only. >>> >>> Thanks, >>> Jie >>> >> >> If the plan is to only check for non-zero trace ID and not check the >> mode any more then you don't need to lock. Maybe lets see what Suzuki >> thinks about returning the last trace ID though as it was his idea to >> add -EINVAL. >> > > Sure. The trace ID is allocated during TPDA device probe and remains > unchanged for the entire lifetime of the device. > > Thanks, > Jie > > Oh I missed that because it's assigned every time in tpdm_enable(). In that case I don't see why it can't be just: val = drvdata->traceid; if (!val) return -EINVAL; return sysfs_emit(...) >>>> Scenarios for reading: >>>> 1. device is enabled -> trace ID is valid >>>> 2. device is enabled then disabled -> trace ID is valid for the last >>>> trace event >>>> 3. device is never enabled -> invalid trace ID (value 0) >>>> >>>> we only need to check the validation of the trace ID. >>>> >>>> mutex_lock(&coresight_mutex); >>>> val = drvdata->traceid; >>>> mutex_unlock(&coresight_mutex); >>>> >>>> if (!val) >>>> return -EINVAL; >>>> >>>> return sysfs_emit(buf, "%#lx\n", val); >>>> >>>> Thanks, >>>> Jie >>>> >>>>> >>>>>> + return sysfs_emit(buf, "%#lx\n", val); >>>>>> +} >>>>>> +static DEVICE_ATTR_RO(traceid); >>>>>> + >>>>>> +static struct attribute *traceid_attrs[] = { >>>>>> + &dev_attr_traceid.attr, >>>>>> + NULL, >>>>>> +}; >>>>>> + >>>>>> +static struct attribute_group traceid_attr_grp = { >>>>>> + .attrs = traceid_attrs, >>>>>> +}; >>>>>> + >>>>>> static ssize_t dsb_mode_show(struct device *dev, >>>>>> struct device_attribute *attr, >>>>>> char *buf) >>>>>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>>>>> *tpdm_attr_grps[] = { >>>>>> &tpdm_cmb_patt_grp, >>>>>> &tpdm_cmb_msr_grp, >>>>>> &tpdm_mcmb_attr_grp, >>>>>> + &traceid_attr_grp, >>>>>> + NULL, >>>>>> +}; >>>>>> + >>>>>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>>>>> + &traceid_attr_grp, >>>>>> NULL, >>>>>> }; >>>>>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>>>>> struct resource *res) >>>>>> desc.access = CSDEV_ACCESS_IOMEM(base); >>>>>> if (res) >>>>>> desc.groups = tpdm_attr_grps; >>>>>> + else >>>>>> + desc.groups = static_tpdm_attr_grps; >>>>>> drvdata->csdev = coresight_register(&desc); >>>>>> if (IS_ERR(drvdata->csdev)) >>>>>> return PTR_ERR(drvdata->csdev); >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/ >>>>>> drivers/ hwtracing/coresight/coresight-tpdm.h >>>>>> index 2867f3ab8186..11da64e1ade8 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>>>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>>>>> * @cmb Specifics associated to TPDM CMB. >>>>>> * @dsb_msr_num Number of MSR supported by DSB TPDM >>>>>> * @cmb_msr_num Number of MSR supported by CMB TPDM >>>>>> + * @traceid Trace ID of the path. >>>>>> */ >>>>>> struct tpdm_drvdata { >>>>>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>>>>> struct cmb_dataset *cmb; >>>>>> u32 dsb_msr_num; >>>>>> u32 cmb_msr_num; >>>>>> + u8 traceid; >>>>>> }; >>>>>> /* Enumerate members of various datasets */ >>>>>> >>>>>> --- >>>>>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>>>>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>>>>> >>>>>> Best regards, >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-31 9:26 ` James Clark 2026-03-31 9:33 ` Jie Gan @ 2026-03-31 9:42 ` Suzuki K Poulose 2026-03-31 9:48 ` Jie Gan 1 sibling, 1 reply; 9+ messages in thread From: Suzuki K Poulose @ 2026-03-31 9:42 UTC (permalink / raw) To: James Clark, Jie Gan Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang On 31/03/2026 10:26, James Clark wrote: > > > On 31/03/2026 4:18 am, Jie Gan wrote: >> Hi James, >> >> On 3/31/2026 9:29 AM, Jie Gan wrote: >>> >>> Hi James, >>> >>> On 3/30/2026 10:55 PM, James Clark wrote: >>>> >>>> >>>> On 25/03/2026 3:10 am, Jie Gan wrote: >>>>> Save the trace ID in drvdata during TPDM enablement and expose it >>>>> to userspace to support trace data parsing. >>>>> >>>>> The TPDM device’s trace ID corresponds to the trace ID allocated >>>>> to the connected TPDA device. >>>>> >>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>>>> --- >>>>> Changes in v3: >>>>> 1. Only allow user to read the traceid while the TPDM device is >>>>> enabled. >>>>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>>>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>>>> >>>>> Changes in V2: >>>>> 1. Use sysfs_emit instead of sprintf. >>>>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- >>>>> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>>>> --- >>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 34 +++++++++++++++ >>>>> + + + +++++++++- >>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >>>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/ >>>>> drivers/ hwtracing/coresight/coresight-tpdm.c >>>>> index da77bdaad0a4..c8339b973bfc 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>>>> *drvdata) >>>>> static int tpdm_enable(struct coresight_device *csdev, struct >>>>> perf_event *event, >>>>> enum cs_mode mode, >>>>> - __maybe_unused struct coresight_path *path) >>>>> + struct coresight_path *path) >>>>> { >>>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev- >>>>> >dev.parent); >>>>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>>>> *csdev, struct perf_event *event, >>>>> } >>>>> __tpdm_enable(drvdata); >>>>> + drvdata->traceid = path->trace_id; >>>>> drvdata->enable = true; >>>>> spin_unlock(&drvdata->spinlock); >>>>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>>>> .attrs = tpdm_attrs, >>>>> }; >>>>> +static ssize_t traceid_show(struct device *dev, >>>>> + struct device_attribute *attr, char *buf) >>>>> +{ >>>>> + unsigned long val; >>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>>> + >>>>> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>>>> + return -EINVAL; >>>>> + >>>>> + val = drvdata->traceid; >>>> >>>> You probably need to take the coresight_mutex here otherwise you >>>> could still return an invalid or stale value despite checking the mode. >>>> >>> >>> Acked. I have missed this potential race condition. >>> >>>> There might also be some value in it returning the last used trace >>>> ID even if the mode isn't enabled anymore. Because you can still >>>> read out of the sink after disabling, so it makes more sense for a >>>> script to read it at that point rather than when it's enabled. Also, >>>> you probably don't want to be doing other things in your script in >>>> the point between enabling and disabling. >>> >>> That's making sense. I shouldnt add such restriction for the read >>> process. >>> >> >> I missed one point in last message. >> >> Is that acceptable to export the coresight_mutex from the core module? >> Currently, the coresight_mutex is used within the module only. >> >> Thanks, >> Jie >> > > If the plan is to only check for non-zero trace ID and not check the > mode any more then you don't need to lock. Maybe lets see what Suzuki > thinks about returning the last trace ID though as it was his idea to > add -EINVAL. > I think there is value in reading out the traceid that was used. Also, I assume TPDM is always connected to a TPDA port and the traceID wouldn't change, ever ? I am happy with the proposal Suzuki >>> Scenarios for reading: >>> 1. device is enabled -> trace ID is valid >>> 2. device is enabled then disabled -> trace ID is valid for the last >>> trace event >>> 3. device is never enabled -> invalid trace ID (value 0) >>> >>> we only need to check the validation of the trace ID. >>> >>> mutex_lock(&coresight_mutex); >>> val = drvdata->traceid; >>> mutex_unlock(&coresight_mutex); >>> >>> if (!val) >>> return -EINVAL; >>> >>> return sysfs_emit(buf, "%#lx\n", val); >>> >>> Thanks, >>> Jie >>> >>>> >>>>> + return sysfs_emit(buf, "%#lx\n", val); >>>>> +} >>>>> +static DEVICE_ATTR_RO(traceid); >>>>> + >>>>> +static struct attribute *traceid_attrs[] = { >>>>> + &dev_attr_traceid.attr, >>>>> + NULL, >>>>> +}; >>>>> + >>>>> +static struct attribute_group traceid_attr_grp = { >>>>> + .attrs = traceid_attrs, >>>>> +}; >>>>> + >>>>> static ssize_t dsb_mode_show(struct device *dev, >>>>> struct device_attribute *attr, >>>>> char *buf) >>>>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>>>> *tpdm_attr_grps[] = { >>>>> &tpdm_cmb_patt_grp, >>>>> &tpdm_cmb_msr_grp, >>>>> &tpdm_mcmb_attr_grp, >>>>> + &traceid_attr_grp, >>>>> + NULL, >>>>> +}; >>>>> + >>>>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>>>> + &traceid_attr_grp, >>>>> NULL, >>>>> }; >>>>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>>>> struct resource *res) >>>>> desc.access = CSDEV_ACCESS_IOMEM(base); >>>>> if (res) >>>>> desc.groups = tpdm_attr_grps; >>>>> + else >>>>> + desc.groups = static_tpdm_attr_grps; >>>>> drvdata->csdev = coresight_register(&desc); >>>>> if (IS_ERR(drvdata->csdev)) >>>>> return PTR_ERR(drvdata->csdev); >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/ >>>>> drivers/ hwtracing/coresight/coresight-tpdm.h >>>>> index 2867f3ab8186..11da64e1ade8 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>>>> * @cmb Specifics associated to TPDM CMB. >>>>> * @dsb_msr_num Number of MSR supported by DSB TPDM >>>>> * @cmb_msr_num Number of MSR supported by CMB TPDM >>>>> + * @traceid Trace ID of the path. >>>>> */ >>>>> struct tpdm_drvdata { >>>>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>>>> struct cmb_dataset *cmb; >>>>> u32 dsb_msr_num; >>>>> u32 cmb_msr_num; >>>>> + u8 traceid; >>>>> }; >>>>> /* Enumerate members of various datasets */ >>>>> >>>>> --- >>>>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>>>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>>>> >>>>> Best regards, >>>> >>> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid 2026-03-31 9:42 ` Suzuki K Poulose @ 2026-03-31 9:48 ` Jie Gan 0 siblings, 0 replies; 9+ messages in thread From: Jie Gan @ 2026-03-31 9:48 UTC (permalink / raw) To: Suzuki K Poulose, James Clark Cc: coresight, linux-arm-kernel, linux-kernel, Mike Leach, Leo Yan, Alexander Shishkin, Tingwei Zhang On 3/31/2026 5:42 PM, Suzuki K Poulose wrote: > On 31/03/2026 10:26, James Clark wrote: >> >> >> On 31/03/2026 4:18 am, Jie Gan wrote: >>> Hi James, >>> >>> On 3/31/2026 9:29 AM, Jie Gan wrote: >>>> >>>> Hi James, >>>> >>>> On 3/30/2026 10:55 PM, James Clark wrote: >>>>> >>>>> >>>>> On 25/03/2026 3:10 am, Jie Gan wrote: >>>>>> Save the trace ID in drvdata during TPDM enablement and expose it >>>>>> to userspace to support trace data parsing. >>>>>> >>>>>> The TPDM device’s trace ID corresponds to the trace ID allocated >>>>>> to the connected TPDA device. >>>>>> >>>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>>>>> --- >>>>>> Changes in v3: >>>>>> 1. Only allow user to read the traceid while the TPDM device is >>>>>> enabled. >>>>>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show- >>>>>> for- tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com >>>>>> >>>>>> Changes in V2: >>>>>> 1. Use sysfs_emit instead of sprintf. >>>>>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid- >>>>>> show- for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/ >>>>>> --- >>>>>> drivers/hwtracing/coresight/coresight-tpdm.c | 34 ++++++++++++++ >>>>>> + + + + +++++++++- >>>>>> drivers/hwtracing/coresight/coresight-tpdm.h | 2 ++ >>>>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/ >>>>>> drivers/ hwtracing/coresight/coresight-tpdm.c >>>>>> index da77bdaad0a4..c8339b973bfc 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c >>>>>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata >>>>>> *drvdata) >>>>>> static int tpdm_enable(struct coresight_device *csdev, struct >>>>>> perf_event *event, >>>>>> enum cs_mode mode, >>>>>> - __maybe_unused struct coresight_path *path) >>>>>> + struct coresight_path *path) >>>>>> { >>>>>> struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev- >>>>>> >dev.parent); >>>>>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device >>>>>> *csdev, struct perf_event *event, >>>>>> } >>>>>> __tpdm_enable(drvdata); >>>>>> + drvdata->traceid = path->trace_id; >>>>>> drvdata->enable = true; >>>>>> spin_unlock(&drvdata->spinlock); >>>>>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = { >>>>>> .attrs = tpdm_attrs, >>>>>> }; >>>>>> +static ssize_t traceid_show(struct device *dev, >>>>>> + struct device_attribute *attr, char *buf) >>>>>> +{ >>>>>> + unsigned long val; >>>>>> + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); >>>>>> + >>>>>> + if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + val = drvdata->traceid; >>>>> >>>>> You probably need to take the coresight_mutex here otherwise you >>>>> could still return an invalid or stale value despite checking the >>>>> mode. >>>>> >>>> >>>> Acked. I have missed this potential race condition. >>>> >>>>> There might also be some value in it returning the last used trace >>>>> ID even if the mode isn't enabled anymore. Because you can still >>>>> read out of the sink after disabling, so it makes more sense for a >>>>> script to read it at that point rather than when it's enabled. >>>>> Also, you probably don't want to be doing other things in your >>>>> script in the point between enabling and disabling. >>>> >>>> That's making sense. I shouldnt add such restriction for the read >>>> process. >>>> >>> >>> I missed one point in last message. >>> >>> Is that acceptable to export the coresight_mutex from the core module? >>> Currently, the coresight_mutex is used within the module only. >>> >>> Thanks, >>> Jie >>> >> >> If the plan is to only check for non-zero trace ID and not check the >> mode any more then you don't need to lock. Maybe lets see what Suzuki >> thinks about returning the last trace ID though as it was his idea to >> add -EINVAL. >> > > I think there is value in reading out the traceid that was used. Also, > I assume TPDM is always connected to a TPDA port and the traceID > wouldn't change, ever ? > Yes. > I am happy with the proposal > James also agrees this proposal in other thread. I will submit a new patch to address it. ABI document missed in this patch also, I will add it in the new patch. Thanks, Jie > Suzuki > >>>> Scenarios for reading: >>>> 1. device is enabled -> trace ID is valid >>>> 2. device is enabled then disabled -> trace ID is valid for the last >>>> trace event >>>> 3. device is never enabled -> invalid trace ID (value 0) >>>> >>>> we only need to check the validation of the trace ID. >>>> >>>> mutex_lock(&coresight_mutex); >>>> val = drvdata->traceid; >>>> mutex_unlock(&coresight_mutex); >>>> >>>> if (!val) >>>> return -EINVAL; >>>> >>>> return sysfs_emit(buf, "%#lx\n", val); >>>> >>>> Thanks, >>>> Jie >>>> >>>>> >>>>>> + return sysfs_emit(buf, "%#lx\n", val); >>>>>> +} >>>>>> +static DEVICE_ATTR_RO(traceid); >>>>>> + >>>>>> +static struct attribute *traceid_attrs[] = { >>>>>> + &dev_attr_traceid.attr, >>>>>> + NULL, >>>>>> +}; >>>>>> + >>>>>> +static struct attribute_group traceid_attr_grp = { >>>>>> + .attrs = traceid_attrs, >>>>>> +}; >>>>>> + >>>>>> static ssize_t dsb_mode_show(struct device *dev, >>>>>> struct device_attribute *attr, >>>>>> char *buf) >>>>>> @@ -1367,6 +1391,12 @@ static const struct attribute_group >>>>>> *tpdm_attr_grps[] = { >>>>>> &tpdm_cmb_patt_grp, >>>>>> &tpdm_cmb_msr_grp, >>>>>> &tpdm_mcmb_attr_grp, >>>>>> + &traceid_attr_grp, >>>>>> + NULL, >>>>>> +}; >>>>>> + >>>>>> +static const struct attribute_group *static_tpdm_attr_grps[] = { >>>>>> + &traceid_attr_grp, >>>>>> NULL, >>>>>> }; >>>>>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, >>>>>> struct resource *res) >>>>>> desc.access = CSDEV_ACCESS_IOMEM(base); >>>>>> if (res) >>>>>> desc.groups = tpdm_attr_grps; >>>>>> + else >>>>>> + desc.groups = static_tpdm_attr_grps; >>>>>> drvdata->csdev = coresight_register(&desc); >>>>>> if (IS_ERR(drvdata->csdev)) >>>>>> return PTR_ERR(drvdata->csdev); >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/ >>>>>> drivers/ hwtracing/coresight/coresight-tpdm.h >>>>>> index 2867f3ab8186..11da64e1ade8 100644 >>>>>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h >>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h >>>>>> @@ -300,6 +300,7 @@ struct cmb_dataset { >>>>>> * @cmb Specifics associated to TPDM CMB. >>>>>> * @dsb_msr_num Number of MSR supported by DSB TPDM >>>>>> * @cmb_msr_num Number of MSR supported by CMB TPDM >>>>>> + * @traceid Trace ID of the path. >>>>>> */ >>>>>> struct tpdm_drvdata { >>>>>> @@ -313,6 +314,7 @@ struct tpdm_drvdata { >>>>>> struct cmb_dataset *cmb; >>>>>> u32 dsb_msr_num; >>>>>> u32 cmb_msr_num; >>>>>> + u8 traceid; >>>>>> }; >>>>>> /* Enumerate members of various datasets */ >>>>>> >>>>>> --- >>>>>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900 >>>>>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00 >>>>>> >>>>>> Best regards, >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-03-31 9:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-25 3:10 [PATCH v3] coresight: tpdm: add traceid_show for checking traceid Jie Gan 2026-03-30 14:55 ` James Clark 2026-03-31 1:29 ` Jie Gan 2026-03-31 3:18 ` Jie Gan 2026-03-31 9:26 ` James Clark 2026-03-31 9:33 ` Jie Gan 2026-03-31 9:44 ` James Clark 2026-03-31 9:42 ` Suzuki K Poulose 2026-03-31 9:48 ` Jie Gan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox