When specifying a 2GB AUX buffer, the ETR driver ends up allocating only
a 1MB buffer instead:
# echo 'file coresight-tmc-etr.c +p' > \
/sys/kernel/debug/dynamic_debug/control
# perf record -e cs_etm/@tmc_etr0,timestamp=0/u -C 0 -m ,2G -- test
coresight tmc_etr0: allocated buffer of size 1024KB in mode 0
The page index is an 'int' type, and shifting it by PAGE_SHIFT overflows
when the resulting value exceeds 2GB. This produces a negative value,
causing the driver to fall back to the minimum buffer size (1MB).
Cast the page index to a wider type to accommodate large buffer sizes.
Also fix a similar issue in the buffer offset calculation.
Reported-by: Michiel van Tol <michiel.vantol(a)arm.com>
Fixes: 99443ea19e8b ("coresight: Add generic TMC sg table framework")
Fixes: eebe8dbd8630 ("coresight: tmc: Decouple the perf buffer allocation from sysfs mode")
Signed-off-by: Leo Yan <leo.yan(a)arm.com>
---
drivers/hwtracing/coresight/coresight-tmc-etr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index cee82e52c4ea96b035f1db71b2d9a006bfc1c51e..990bbb721e1d712d7b93f1e36087fdaf9d3baa3b 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -154,7 +154,7 @@ tmc_pages_get_offset(struct tmc_pages *tmc_pages, dma_addr_t addr)
for (i = 0; i < tmc_pages->nr_pages; i++) {
page_start = tmc_pages->daddrs[i];
if (addr >= page_start && addr < (page_start + PAGE_SIZE))
- return i * PAGE_SIZE + (addr - page_start);
+ return (long)i * PAGE_SIZE + (addr - page_start);
}
return -EINVAL;
@@ -1381,7 +1381,7 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,
node = (event->cpu == -1) ? NUMA_NO_NODE : cpu_to_node(event->cpu);
/* Use the minimum limit if the required size is smaller */
- size = nr_pages << PAGE_SHIFT;
+ size = (ssize_t)nr_pages << PAGE_SHIFT;
size = max_t(ssize_t, size, TMC_ETR_PERF_MIN_BUF_SIZE);
/*
---
base-commit: eebe8dbd8630f51cf70b1f68a440cd3d7f7a914d
change-id: 20260217-arm_coresight_fix_big_buffer_size-a8a41298369d
Best regards,
--
Leo Yan <leo.yan(a)arm.com>
Fix PM save on ETE, which is an issue that showed up on the FVP when
booted with ACPI and the newly enabled idle states. Then refactor to
clarify and avoid any probe order issues.
Signed-off-by: James Clark <james.clark(a)linaro.org>
---
James Clark (2):
coresight: ete: Always save state on power down
coresight: etm4x: Refactor pm_save_enable handling
drivers/hwtracing/coresight/coresight-etm4x-core.c | 55 ++++++++++++++++------
drivers/hwtracing/coresight/coresight-etm4x.h | 1 +
2 files changed, 42 insertions(+), 14 deletions(-)
---
base-commit: 971f3474f8898ae8bbab19a9b547819a5e6fbcf1
change-id: 20260420-james-cs-ete-pm_save_enable-4e994e35cdac
Best regards,
--
James Clark <james.clark(a)linaro.org>
On Tue, Apr 28, 2026 at 10:25:11AM +0800, Yingchao Deng (Consultant) wrote:
[...]
> > tg->used_mask = bitmap_zalloc(nr_filter_sigs, GFP_KERNEL);
> "nr_filter_sigs" is the count of entries in the DT property array, if the DT
> property is:
> arm,trig-filters = <22 23>;
> Here nr_filter_sigs=2, so bitmap_zalloc(2) allocates only 1 unsigned long
> (64 bits). set_bit(22/23, used_mask) still fits, but it's logically an OOB,
> and any index >=64 would
> write past the end.
Thanks for explanation. It is correct for me.
On Sun, Apr 26, 2026 at 05:44:39PM +0800, Yingchao Deng wrote:
> Introduce a small encoding to carry the register index together with the
> base offset in a single u32, and use a common helper to compute the final
> MMIO address. This refactors register access to be based on the encoded
> (reg, nr) pair, reducing duplicated arithmetic and making it easier to
> support variants that bank or relocate trigger-indexed registers.
>
> Signed-off-by: Yingchao Deng <yingchao.deng(a)oss.qualcomm.com>
> ---
> drivers/hwtracing/coresight/coresight-cti-core.c | 31 +++++++++++++++--------
> drivers/hwtracing/coresight/coresight-cti-sysfs.c | 4 +--
> drivers/hwtracing/coresight/coresight-cti.h | 16 ++++++++++--
> 3 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index 4e7d12bd2d3e..c4cbeb64365b 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -42,6 +42,14 @@ static DEFINE_MUTEX(ect_mutex);
> #define csdev_to_cti_drvdata(csdev) \
> dev_get_drvdata(csdev->dev.parent)
>
> +static void __iomem *cti_reg_addr(struct cti_drvdata *drvdata, int reg)
> +{
> + u32 offset = CTI_REG_CLR_NR(reg);
> + u32 nr = CTI_REG_GET_NR(reg);
> +
> + return drvdata->base + offset + sizeof(u32) * nr;
> +}
Could you try below change, which is more straightforward?
static void __iomem *__reg_addr(struct cti_drvdata *drvdata, int off,
int index)
{
return drvdata->base + offset + sizeof(u32) * index;
}
#define reg_addr(drvdata, off) \
__reg_addr((drvdata), (off), 0)
#define reg_index_addr(drvdata, off, i) \
__reg_addr((drvdata), (off), (i))
> +
> /* write set of regs to hardware - call with spinlock claimed */
> void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
> {
> @@ -55,16 +63,17 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
>
> /* write the CTI trigger registers */
> for (i = 0; i < config->nr_trig_max; i++) {
> - writel_relaxed(config->ctiinen[i], drvdata->base + CTIINEN(i));
> + writel_relaxed(config->ctiinen[i],
> + cti_reg_addr(drvdata, CTI_REG_SET_NR(CTIINEN, i)));
writel_relaxed(config->ctiinen[i],
reg_index_addr(drvdata, CTIINEN, i));
> writel_relaxed(config->ctiouten[i],
> - drvdata->base + CTIOUTEN(i));
> + cti_reg_addr(drvdata, CTI_REG_SET_NR(CTIOUTEN, i)));
writel_relaxed(config->ctiouten[i],
reg_index_addr(drvdata, CTIOUTEN, i));
[...]
> +/*
> + * Encode CTI register offset and register index in one u32:
> + * - bits[0:11] : base register offset (0x000 to 0xFFF)
> + * - bits[24:31] : register index (nr)
> + */
> +#define CTI_REG_NR_MASK GENMASK(31, 24)
> +#define CTI_REG_GET_NR(reg) FIELD_GET(CTI_REG_NR_MASK, (reg))
> +#define CTI_REG_SET_NR_CONST(reg, nr) ((reg) | FIELD_PREP_CONST(CTI_REG_NR_MASK, (nr)))
> +#define CTI_REG_SET_NR(reg, nr) ((reg) | FIELD_PREP(CTI_REG_NR_MASK, (nr)))
> +#define CTI_REG_CLR_NR(reg) ((reg) & (~CTI_REG_NR_MASK))
I know this might come from my suggestion, and it is also will be
heavily used in patch 04. We can have strightforward way to
implement this, please drop these macros.
I will reply in patch 04 separately. Sorry my review might cause
extra effort.
Thanks,
Leo
On Sun, Apr 26, 2026 at 05:44:38PM +0800, Yingchao Deng wrote:
[...]
> @@ -316,23 +316,33 @@ static int cti_plat_process_filter_sigs(struct cti_drvdata *drvdata,
> {
> struct cti_trig_grp *tg = NULL;
> int err = 0, nr_filter_sigs;
> + int nr_trigs = drvdata->config.nr_trig_max;
>
> nr_filter_sigs = cti_plat_count_sig_elements(fwnode,
> CTI_DT_FILTER_OUT_SIGS);
> if (nr_filter_sigs == 0)
> return 0;
>
> - if (nr_filter_sigs > drvdata->config.nr_trig_max)
> + if (nr_filter_sigs > nr_trigs)
> return -EINVAL;
>
> tg = kzalloc_obj(*tg);
> if (!tg)
> return -ENOMEM;
>
> + tg->used_mask = bitmap_zalloc(nr_trigs, GFP_KERNEL);
Here would be:
tg->used_mask = bitmap_zalloc(nr_filter_sigs, GFP_KERNEL);
> + if (!tg->used_mask) {
> + kfree(tg);
> + return -ENOMEM;
> + }
> +
It is likely this will have merge conflict with the new patch [1].
You might need to rebase this patch on the top of [1]. We need to
give [1] priority as it is a fix.
[1] https://lore.kernel.org/linux-arm-kernel/20260426-nr_sigs-v1-1-3b9df99dab97…
Otherwise, LGTM:
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On Sun, Apr 26, 2026 at 05:59:34PM +0800, Yingchao Deng wrote:
> In cti_plat_process_filter_sigs(), after allocating a temporary
> cti_trig_grp struct via kzalloc_obj(), the code never assigns tg->nr_sigs
> = nr_filter_sigs. Since kzalloc zero-initialises the struct, tg->nr_sigs
> remains 0. cti_plat_read_trig_group() guards with:
> if (!tgrp->nr_sigs)
> return 0;
>
> so it returns immediately without reading any signal indices from DT.
>
> Fix by assigning tg->nr_sigs before calling cti_plat_read_trig_group().
>
> Fixes: a5614770ab97 ("coresight: cti: Add device tree support for custom CTI")
> Signed-off-by: Yingchao Deng <yingchao.deng(a)oss.qualcomm.com>
> ---
> drivers/hwtracing/coresight/coresight-cti-platform.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c
> index 4eff96f48594..d6d5388705c3 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c
> @@ -329,6 +329,7 @@ static int cti_plat_process_filter_sigs(struct cti_drvdata *drvdata,
> if (!tg)
> return -ENOMEM;
>
> + tg->nr_sigs = nr_filter_sigs;
> err = cti_plat_read_trig_group(tg, fwnode, CTI_DT_FILTER_OUT_SIGS);
I checked the naming, seems tg->nr_sigs is the right field to store
the number. LGTM:
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
Hi,
On 4/21/26 11:30, Yeoreum Yun wrote:
>> Hi Mike,
>>
>>> Hi,
>>>
>>> This register [bit 31] indicates if a single shot comparator has matched. So
>>> read-back provides information to the user post run to determine which if
>>> any of the comparators set in this way has actually matched.
>>
>> Okay. so after disable sysfs session, to check former session
>> check whether comprator has matched.
>>
>>>
>>> Moreover, the specification states "Software must reset this bit to 0 to
>>> re-enable single-shot control" and "Reset state is unknown. STATUS must be
>>> written to set an initial state...."
>>>
>>> Therefore this register must be written as part of any configuration so
>>> should be available in the drvdata->config for both read and write,
>>
>> But I don't think this is the reason for locate ss_status into "config"
>> since its write purpose is not to configure but the "clear" former bit.
>> That's why I think it's enough to clear when the new sysfs session starts.
>>
>
> IOW, I think it's better to remove ss_status from configfs item
> and
> - add field ss_cmp in etm4_cpas
> - add another field ss_status under "etm4_drvdata" to show "PENDING
> and STATUS" bits to sysfs after finishing session.
>
> Is is valid for you?
>
No. Why two different locations for a single register read? If I have
the ETMv4 hardware manual I am going to look for a something that is
recognizable as being related to the single shot comparator status
register(s).
So in sysfs I would expect to see all the bits from the register,
displayed, without masking off the STATUS and PENDING bits as happens now.
In the code I would expect to see a single location with a sensible name
- ss_cmp doesn't really correlate terribly well with TRCSSCSR. If you do
not like the original ss_status, then ss_cmp_status may actually be
better, ss_cmp could be either the ss comparator status or control
register.
Regards
Mike
>> --
>> Sincerely,
>> Yeoreum Yun
>>
>
> --
> Sincerely,
> Yeoreum Yun