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

[James Clark <james.clark@linaro.org>]

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,
>>>>>
>>>>
>>>
>>
>