Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid

[Jie Gan]

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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx
> >
> > 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@xxxxxxxxxxxxxxxx/
> > ---
> >   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,