Hi Jie,
On Fri, Apr 17, 2026 at 09:01:17AM +0800, Jie Gan wrote:
[...]
> ... we still need support static trace ID allocation in parallel for
> the dummy sources and we should not break this logic in future refactor.
Just confirm what is the reason you need to use static trace ID for the
dummy sources?
I am wandering if we could use dev->devt as trace ID for dummy
devices. Since the device's MAJOR number is non-zero and occupies the
upper bits (see MINORBITS), it is naturally separated from the hardware
trace ID range. If so, we even don't need to bother ID alloc/release.
Thanks,
Leo
Hi Levi,
On Thu, Apr 16, 2026 at 06:06:41PM +0100, Yeoreum Yun wrote:
[...]
> > We should use paired way for allocation and release. For example:
> >
> > coresight_enable_sysfs()
> > {
> > ...
> > coresight_path_assign_trace_id(path);
> >
> > failed:
> > coresight_path_unassign_trace_id(path);
> > }
> >
> > coresight_disable_sysfs()
> > {
> > coresight_path_unassign_trace_id(path);
> > }
> >
> > But this requires broader refactoring. E.g., the STM driver currently
> > allocates system trace IDs statically during probe, we might need to
> > consolidate for all modules to use dynamic allocation.
>
> So IIUC, Do we want to "map" per "session" and save this map information
> in the "sink" driver? or just use "global" map but locate it in sink
> driver?
I prefer to save map in the sink's driver data, this is more scalable
as the trace ID is allocated within a session rather than system wide.
> I totally agree for above suggestion -- unsigned trace id
> in the coresight_XXX function -- (but we need to add another callback
> for this) but I think we don't need to sustain map per session
> and it seems enough to use current storage for trace_id not move to
> sink driver.
>
> Anyway It would be better to refactorying wiht another patchset...
Yeah, we can come back to these ideas when work on it.
Thanks,
Leo
On Wed, Apr 15, 2026 at 05:55:22PM +0100, Yeoreum Yun wrote:
> If etm4_enable_sysfs() fails in cscfg_csdev_enable_active_config(),
> the trace ID may be leaked because it is not released.
>
> To address this, call etm4_release_trace_id() when etm4_enable_sysfs()
> fails in cscfg_csdev_enable_active_config().
>
> Reviewed-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
> Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com>
> ---
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index f55338a4989d..b199aebbdb60 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -920,8 +920,10 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
> if (cfg_hash) {
> ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
> - if (ret)
> + if (ret) {
> + etm4_release_trace_id(drvdata);
> return ret;
> + }
LGTM:
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
Just recording a bit thoughts. As Suzuki mentioned, it would be better
to allocate trace IDs within a session. We might consider maintaining
the trace ID map in the sink driver data, since the sink driver is
unique within a session so it is a central place to allocate trace ID.
We should use paired way for allocation and release. For example:
coresight_enable_sysfs()
{
...
coresight_path_assign_trace_id(path);
failed:
coresight_path_unassign_trace_id(path);
}
coresight_disable_sysfs()
{
coresight_path_unassign_trace_id(path);
}
But this requires broader refactoring. E.g., the STM driver currently
allocates system trace IDs statically during probe, we might need to
consolidate for all modules to use dynamic allocation.
Thanks,
Leo
On Wed, Apr 15, 2026 at 05:55:20PM +0100, Yeoreum Yun wrote:
[...]
> @@ -573,11 +573,9 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
>
> for (i = 0; i < caps->nr_ss_cmp; i++) {
> - /* always clear status bit on restart if using single-shot */
> - if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> - config->ss_status[i] &= ~TRCSSCSRn_STATUS;
> etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
> - etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
> + /* always clear status and pending bits on restart if using single-shot */
> + etm4x_relaxed_write32(csa, 0x0, TRCSSCSRn(i));
In Arm ARM, D24.4.60 TRCSSCSR<n>, bits[0..3] are RO. I think it is
fine for directly clear the regiser with zero (means it will only
clear status / pending bits).
[...]
> @@ -1841,10 +1839,11 @@ static ssize_t sshot_status_show(struct device *dev,
> {
> unsigned long val;
> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
> + const struct etmv4_caps *caps = &drvdata->caps;
> struct etmv4_config *config = &drvdata->config;
>
> raw_spin_lock(&drvdata->spinlock);
> - val = config->ss_status[config->ss_idx];
> + val = caps->ss_cmp[config->ss_idx];
> raw_spin_unlock(&drvdata->spinlock);
> return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
> }
This sysfs knob never can print out a realtime status for sshot, I am
fine for only printing caps->ss_cmp, this can avoid any misleading.
@Suzuki, @Mike, do you agree with the change above?
If maintainers agree with this, as Jie suggested, it is good to add a
comment in the code and update the document:
Documentation/trace/coresight/coresight-etm4x-reference.rst
Thanks,
Leo
On Wed, Apr 15, 2026 at 05:55:18PM +0100, Yeoreum Yun wrote:
> TCRSEQEVR<n> is implemented only when TCRIDR5.NUMSEQSTATE is 0b100,
> in which case n ranges from 0 to 2; otherwise, TCRIDR5.NUMSEQSTATE is 0b000.
My suggestion in previous version is not quite right, thanks for
making this correct.
[...]
> @@ -1395,6 +1395,8 @@ static ssize_t seq_idx_store(struct device *dev,
> struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
> struct etmv4_config *config = &drvdata->config;
>
> + if (!drvdata->nrseqstate)
> + return -EINVAL;
For "nrseqstate = 0" case, would it return -EOPNOTSUPP instead?
Otherwise, LGTM:
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On Wed, Apr 15, 2026 at 05:55:17PM +0100, Yeoreum Yun wrote:
> According to Embedded Trace Macrocell Architecture Specification
> ETMv4.0 to ETM4.6 [0], TRCSSPCICR<n> is present only if all of
> the following are true:
>
> - TRCIDR4.NUMSSCC > n.
> - TRCIDR4.NUMPC > 0b0000.
> - TRCSSCSR<n>.PC == 0b1.
>
> Comment for etm4x_sspcicrn_present() is align with the specification.
> However, the check should use drvdata->nr_pe_cmp to check TRCIDR4.NUMPC
> not nr_pe.
>
> Link: https://developer.arm.com/documentation/ihi0064/latest/ [0]
> Signed-off-by: Yeoreum Yun <yeoreum.yun(a)arm.com>
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On 15/04/2026 09:45, Jie Gan wrote:
>
>
> On 4/15/2026 4:32 PM, Leo Yan wrote:
>> On Wed, Apr 15, 2026 at 09:01:09AM +0100, Yeoreum Yun wrote:
>>
>> [...]
>>
>>>>> What I am thinking is as SoCs continue to grow more complex with an
>>>>> increasing number of subsystems, trace IDs may be exhausted in the
>>>>> near
>>>>> future. (that's why we have dynamic trace ID allocation/release).
>>>>
>>>> Thanks for the input.
>>>>
>>>> I am wandering if we can use "dev->devt" as the trace ID. A device's
>>>> major/minor number is unique in kernel and dev_t is defined as u32:
>>>>
>>>> typedef u32 __kernel_dev_t;
>>>>
>>>> And we can consolidate this for both SYSFS and PERF modes.
>>>>
>>>
>>> When I see the CORESIGHT_TRACE_ID_MAX:
>>>
>>> /* architecturally we have 128 IDs some of which are reserved */
>>> #define CORESIGHT_TRACE_IDS_MAX 128
>>>
>>> I think this came from the hardware restriction for number of TRACE_IDs.
>>> In this case, clamping the device_id to trace_id seems more complex and
>>> reduce some performance perspective.
>>
>> Sigh, my stupid. Please ignore my previous comment, let us first fix
>> ID leak issue.
>>
>> Given Jie's comment on the use-out issue, it is valid for me especially
>> if a system have many dummy tracers. We can defer to refactor it
>> later (e.g., use separate ranges for hardware and dummy tracers).
>>
>> thanks for correction!
>
> Just share some info:
>
> With my memory, The ARM AMBA ATB Protocol Specification defined a 7-bit
> width field for the trace ID, that's where the 128 comes from. (in each
> frame, we also have 7-bit field for containing the trace ID)
That is true and some IDs in the range (0-128) are reserved. So we
actually have less than 128. We need the dynamic allocation, preferrably
isolated to a "pool" for the relevant session to make the full use of
the space.
Suzuki
>
> Thanks,
> Jie
>
On Wed, Apr 15, 2026 at 09:01:09AM +0100, Yeoreum Yun wrote:
[...]
> > > What I am thinking is as SoCs continue to grow more complex with an
> > > increasing number of subsystems, trace IDs may be exhausted in the near
> > > future. (that's why we have dynamic trace ID allocation/release).
> >
> > Thanks for the input.
> >
> > I am wandering if we can use "dev->devt" as the trace ID. A device's
> > major/minor number is unique in kernel and dev_t is defined as u32:
> >
> > typedef u32 __kernel_dev_t;
> >
> > And we can consolidate this for both SYSFS and PERF modes.
> >
>
> When I see the CORESIGHT_TRACE_ID_MAX:
>
> /* architecturally we have 128 IDs some of which are reserved */
> #define CORESIGHT_TRACE_IDS_MAX 128
>
> I think this came from the hardware restriction for number of TRACE_IDs.
> In this case, clamping the device_id to trace_id seems more complex and
> reduce some performance perspective.
Sigh, my stupid. Please ignore my previous comment, let us first fix
ID leak issue.
Given Jie's comment on the use-out issue, it is valid for me especially
if a system have many dummy tracers. We can defer to refactor it
later (e.g., use separate ranges for hardware and dummy tracers).
thanks for correction!
Hi Yingchao,
On Wed, Apr 15, 2026 at 11:22:49AM +0800, Yingchao Deng (Consultant) wrote:
[...]
> Gentle reminder.
This series would be on Mike's radar.
I will also look into details once I finish Levi's series review.
Thanks,
Leo
On Wed, Apr 15, 2026 at 09:21:21AM +0800, Jie Gan wrote:
[...]
> > > > @@ -918,8 +918,10 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
> > > > cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
> > > > if (cfg_hash) {
> > > > ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
> > > > - if (ret)
> > > > + if (ret) {
> > > > + etm4_release_trace_id(drvdata);
> > >
> > > If so, even an ID is reserved for failures, and the ID map is big enough
> > > for each CPU, we don't need to worry memory leak or ID used out issue ?
> >
> > However, in theory, this could lead to an ID leak,
> > so it would be better to release it in error cases.
>
> What I am thinking is as SoCs continue to grow more complex with an
> increasing number of subsystems, trace IDs may be exhausted in the near
> future. (that's why we have dynamic trace ID allocation/release).
Thanks for the input.
I am wandering if we can use "dev->devt" as the trace ID. A device's
major/minor number is unique in kernel and dev_t is defined as u32:
typedef u32 __kernel_dev_t;
And we can consolidate this for both SYSFS and PERF modes.
Thanks,
Leo
