CoreSight April 2025

coresight@lists.linaro.org

10 participants
54 discussions

Re: [PATCH v4 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

by Leo Yan

Hi Yabin, Sorry for late reply as I was on vacation for the past two weeks. On Tue, Apr 15, 2025 at 11:46:48AM -0700, Yabin Cui wrote: > When tracing ETM data on multiple CPUs concurrently via the > perf interface, the CATU device is shared across different CPU > paths. This can lead to race conditions when multiple CPUs attempt > to enable or disable the CATU device simultaneously. > > To address these race conditions, this patch introduces the > following changes: > > 1. The enable and disable operations for the CATU device are not > reentrant. Therefore, a spinlock is added to ensure that only > one CPU can enable or disable a given CATU device at any point > in time. > > 2. A reference counter is used to manage the enable/disable state > of the CATU device. The device is enabled when the first CPU > requires it and is only disabled when the last CPU finishes > using it. This ensures the device remains active as long as at > least one CPU needs it. > > Signed-off-by: Yabin Cui <yabinc(a)google.com> LGTM: Reviewed-by: Leo Yan <leo.yan(a)arm.com> As Jie reminded, please add James' review tag in next spin. Thanks, Leo > --- > drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++------- > drivers/hwtracing/coresight/coresight-catu.h | 1 + > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c > index fa170c966bc3..3909b562b077 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, > static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, > void *data) > { > - int rc; > + int rc = 0; > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); > > - CS_UNLOCK(catu_drvdata->base); > - rc = catu_enable_hw(catu_drvdata, mode, data); > - CS_LOCK(catu_drvdata->base); > + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); > + if (csdev->refcnt == 0) { > + CS_UNLOCK(catu_drvdata->base); > + rc = catu_enable_hw(catu_drvdata, mode, data); > + CS_LOCK(catu_drvdata->base); > + } > + if (!rc) > + csdev->refcnt++; > return rc; > } > > @@ -486,12 +491,15 @@ static int catu_disable_hw(struct catu_drvdata *drvdata) > > static int catu_disable(struct coresight_device *csdev, void *__unused) > { > - int rc; > + int rc = 0; > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); > > - CS_UNLOCK(catu_drvdata->base); > - rc = catu_disable_hw(catu_drvdata); > - CS_LOCK(catu_drvdata->base); > + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); > + if (--csdev->refcnt == 0) { > + CS_UNLOCK(catu_drvdata->base); > + rc = catu_disable_hw(catu_drvdata); > + CS_LOCK(catu_drvdata->base); > + } > return rc; > } > > @@ -550,6 +558,7 @@ static int __catu_probe(struct device *dev, struct resource *res) > dev->platform_data = pdata; > > drvdata->base = base; > + raw_spin_lock_init(&drvdata->spinlock); > catu_desc.access = CSDEV_ACCESS_IOMEM(base); > catu_desc.pdata = pdata; > catu_desc.dev = dev; > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h > index 141feac1c14b..755776cd19c5 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.h > +++ b/drivers/hwtracing/coresight/coresight-catu.h > @@ -65,6 +65,7 @@ struct catu_drvdata { > void __iomem *base; > struct coresight_device *csdev; > int irq; > + raw_spinlock_t spinlock; > }; > > #define CATU_REG32(name, offset) \ > -- > 2.49.0.604.gff1f9ca942-goog >

2 weeks, 5 days

Re: [PATCH 2/2] coresight: etm-perf: Add AUX_NON_CONTIGUOUS_PAGES to cs_etm PMU

by Leo Yan

On Mon, Apr 21, 2025 at 02:58:18PM -0700, Yabin Cui wrote: > The cs_etm PMU, regardless of the underlying trace sink (ETF, ETR or > TRBE), doesn't require contiguous pages for its AUX buffer. Though contiguous pages are not mandatory for TRBE, I would set the PERF_PMU_CAP_AUX_NO_SG flag for it. This can potentially benefit performance. For non per CPU sinks, it is fine to allocate non-contiguous pages. Thanks, Leo > This patch adds the PERF_PMU_CAP_AUX_NON_CONTIGUOUS_PAGES capability > to the cs_etm PMU. This allows the kernel to allocate non-contiguous > pages for the AUX buffer, reducing memory fragmentation when using > cs_etm. > > Signed-off-by: Yabin Cui <yabinc(a)google.com> > --- > drivers/hwtracing/coresight/coresight-etm-perf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index f4cccd68e625..c98646eca7f8 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -899,7 +899,8 @@ int __init etm_perf_init(void) > int ret; > > etm_pmu.capabilities = (PERF_PMU_CAP_EXCLUSIVE | > - PERF_PMU_CAP_ITRACE); > + PERF_PMU_CAP_ITRACE | > + PERF_PMU_CAP_AUX_NON_CONTIGUOUS_PAGES); > > etm_pmu.attr_groups = etm_pmu_attr_groups; > etm_pmu.task_ctx_nr = perf_sw_context; > -- > 2.49.0.805.g082f7c87e0-goog > >

2 weeks, 6 days

Re: [PATCH 1/2] perf: Allow non-contiguous AUX buffer pages via PMU capability

by James Clark

On 21/04/2025 10:58 pm, Yabin Cui wrote: > For PMUs like ARM ETM/ETE, contiguous AUX buffers are unnecessary > and increase memory fragmentation. > > This patch introduces PERF_PMU_CAP_AUX_NON_CONTIGUOUS_PAGES, allowing > PMUs to request non-contiguous pages for their AUX buffers. > > Signed-off-by: Yabin Cui <yabinc(a)google.com> > --- > include/linux/perf_event.h | 1 + > kernel/events/ring_buffer.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 0069ba6866a4..26ca35d6a9f2 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -301,6 +301,7 @@ struct perf_event_pmu_context; > #define PERF_PMU_CAP_AUX_OUTPUT 0x0080 > #define PERF_PMU_CAP_EXTENDED_HW_TYPE 0x0100 > #define PERF_PMU_CAP_AUX_PAUSE 0x0200 > +#define PERF_PMU_CAP_AUX_NON_CONTIGUOUS_PAGES 0x0400 > > /** > * pmu::scope > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > index 5130b119d0ae..87f42f4e8edc 100644 > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -710,6 +710,12 @@ int rb_alloc_aux(struct perf_buffer *rb, struct perf_event *event, > max_order = ilog2(nr_pages); > watermark = 0; > } > + /* > + * When the PMU doesn't prefer contiguous AUX buffer pages, favor > + * low-order allocations to reduce memory fragmentation. > + */ > + if (event->pmu->capabilities & PERF_PMU_CAP_AUX_NON_CONTIGUOUS_PAGES) > + max_order = 0; > > /* > * kcalloc_node() is unable to allocate buffer if the size is larger Hi Yabin, I was wondering if this is just the opposite of PERF_PMU_CAP_AUX_NO_SG, and that order 0 should be used by default for all devices to solve the issue you describe. Because we already have PERF_PMU_CAP_AUX_NO_SG for devices that need contiguous pages. Then I found commit 5768402fd9c6 ("perf/ring_buffer: Use high order allocations for AUX buffers optimistically") that explains that the current allocation strategy is an optimization. Your change seems to decide that for certain devices we want to optimize for fragmentation rather than performance. If these are rarely used features specifically when looking at performance should we not continue to optimize for performance? Or at least make it user configurable? Thanks James

2 weeks, 6 days

Re: [PATCH 3/4] coresight: tmc: refactor the tmc-etr mode setting

by hejunhao

On 2025/4/18 15:12, Jie Gan wrote: > > > On 4/18/2025 1:58 PM, Junhao He wrote: >> When trying to run perf and sysfs mode simultaneously, the WARN_ON() >> in tmc_etr_enable_hw() is triggered sometimes: >> >> WARNING: CPU: 42 PID: 3911571 at >> drivers/hwtracing/coresight/coresight-tmc-etr.c:1060 >> tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] >> [..snip..] >> Call trace: >> tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] (P) >> tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] (L) >> tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] >> coresight_enable_path+0x1c8/0x218 [coresight] >> coresight_enable_sysfs+0xa4/0x228 [coresight] >> enable_source_store+0x58/0xa8 [coresight] >> dev_attr_store+0x20/0x40 >> sysfs_kf_write+0x4c/0x68 >> kernfs_fop_write_iter+0x120/0x1b8 >> vfs_write+0x2c8/0x388 >> ksys_write+0x74/0x108 >> __arm64_sys_write+0x24/0x38 >> el0_svc_common.constprop.0+0x64/0x148 >> do_el0_svc+0x24/0x38 >> el0_svc+0x3c/0x130 >> el0t_64_sync_handler+0xc8/0xd0 >> el0t_64_sync+0x1ac/0x1b0 >> ---[ end trace 0000000000000000 ]--- >> >> Since the sysfs buffer allocation and the hardware enablement is not >> in the same critical region, it's possible to race with the perf >> >> mode: >> [sysfs mode] [perf mode] >> tmc_etr_get_sysfs_buffer() >> spin_lock(&drvdata->spinlock) >> [sysfs buffer allocation] >> spin_unlock(&drvdata->spinlock) >> spin_lock(&drvdata->spinlock) >> tmc_etr_enable_hw() >> drvdata->etr_buf = etr_perf->etr_buf >> spin_unlock(&drvdata->spinlock) >> spin_lock(&drvdata->spinlock) >> tmc_etr_enable_hw() >> WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at >> the perf side >> spin_unlock(&drvdata->spinlock) >> >> To resolve this, configure the tmc-etr mode before invoking >> `enable_perf()` or sysfs interfaces. Prior to mode configuration, >> explicitly check if the tmc-etr sink is already enabled in a >> different mode to prevent race conditions between mode transitions. >> Furthermore, enforce spinlock protection around the critical >> sections to serialize concurrent accesses from sysfs and perf >> subsystems. > > Is any issue observed during this race condition? > > The etr_buf is checked before assign it to sysfs_buf or perf_buf. > In my understanding, the warning raised to indicate the etr already > enabled with sysfs mode or perf mode, so terminate current enable > session, right? > > Thanks, > Jie > The sysfs task is executing the process to enable tmc-etr and has allocated a sysfs_buf. However, the sysfs spinlock does not ensure atomicity of the entire enable_etr_sink_sysfs() function execution. In this scenario, the perf session may preemptively call tmc_etr_enable_hw() to enable tmc-etr. Consequently, during race conditions, perf always prioritizes execution, and the sysfs_buf remains un-released, leading to memory leaks. Then this triggers a WARN_ON(drvdata->etr_buf) warning. This patch also addresses the issue described in [1], simplifying the setup and checks for "mode". [1] Closes: https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-2-yangyicong@… Best regards, Junhao. >> >> Fixes: 296b01fd106e ("coresight: Refactor out buffer allocation >> function for ETR") >> Reported-by: Yicong Yang <yangyicong(a)hisilicon.com> >> Closes: >> https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-2-yangyicong@… >> Signed-off-by: Junhao He <hejunhao3(a)huawei.com> >> --- >> .../hwtracing/coresight/coresight-tmc-etr.c | 77 +++++++++++-------- >> 1 file changed, 47 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c >> b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> index a48bb85d0e7f..3d94d64cacaa 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> @@ -1190,11 +1190,6 @@ static struct etr_buf >> *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) >> spin_lock_irqsave(&drvdata->spinlock, flags); >> } >> - if (drvdata->reading || coresight_get_mode(csdev) == >> CS_MODE_PERF) { >> - ret = -EBUSY; >> - goto out; >> - } >> - >> /* >> * If we don't have a buffer or it doesn't match the requested >> size, >> * use the buffer allocated above. Otherwise reuse the existing >> buffer. >> @@ -1205,7 +1200,6 @@ static struct etr_buf >> *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) >> drvdata->sysfs_buf = new_buf; >> } >> -out: >> spin_unlock_irqrestore(&drvdata->spinlock, flags); >> /* Free memory outside the spinlock if need be */ >> @@ -1216,7 +1210,7 @@ static struct etr_buf >> *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev) >> static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) >> { >> - int ret = 0; >> + int ret; >> unsigned long flags; >> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >> struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev); >> @@ -1226,23 +1220,10 @@ static int tmc_enable_etr_sink_sysfs(struct >> coresight_device *csdev) >> spin_lock_irqsave(&drvdata->spinlock, flags); >> - /* >> - * In sysFS mode we can have multiple writers per sink. Since this >> - * sink is already enabled no memory is needed and the HW need >> not be >> - * touched, even if the buffer size has changed. >> - */ >> - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { >> - csdev->refcnt++; >> - goto out; >> - } >> - >> ret = tmc_etr_enable_hw(drvdata, sysfs_buf); >> - if (!ret) { >> - coresight_set_mode(csdev, CS_MODE_SYSFS); >> + if (!ret) >> csdev->refcnt++; >> - } >> -out: >> spin_unlock_irqrestore(&drvdata->spinlock, flags); >> if (!ret) >> @@ -1652,11 +1633,6 @@ static int tmc_enable_etr_sink_perf(struct >> coresight_device *csdev, void *data) >> struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); >> spin_lock_irqsave(&drvdata->spinlock, flags); >> - /* Don't use this sink if it is already claimed by sysFS */ >> - if (coresight_get_mode(csdev) == CS_MODE_SYSFS) { >> - rc = -EBUSY; >> - goto unlock_out; >> - } >> if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) { >> rc = -EINVAL; >> @@ -1685,7 +1661,6 @@ static int tmc_enable_etr_sink_perf(struct >> coresight_device *csdev, void *data) >> if (!rc) { >> /* Associate with monitored process. */ >> drvdata->pid = pid; >> - coresight_set_mode(csdev, CS_MODE_PERF); >> drvdata->perf_buf = etr_perf->etr_buf; >> csdev->refcnt++; >> } >> @@ -1698,14 +1673,56 @@ static int tmc_enable_etr_sink_perf(struct >> coresight_device *csdev, void *data) >> static int tmc_enable_etr_sink(struct coresight_device *csdev, >> enum cs_mode mode, void *data) >> { >> + struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >> + enum cs_mode old_mode; >> + int rc; >> + >> + scoped_guard(spinlock_irqsave, &drvdata->spinlock) { >> + old_mode = coresight_get_mode(csdev); >> + if (old_mode != CS_MODE_DISABLED && old_mode != mode) >> + return -EBUSY; >> + >> + if (drvdata->reading) >> + return -EBUSY; >> + >> + /* >> + * In sysFS mode we can have multiple writers per sink. >> Since this >> + * sink is already enabled no memory is needed and the HW >> need not be >> + * touched, even if the buffer size has changed. >> + */ >> + if (old_mode == CS_MODE_SYSFS) { >> + csdev->refcnt++; >> + return 0; >> + } >> + >> + /* >> + * minor note: >> + * When sysfs-task1 get locked, it setup the mode first. Then >> + * sysfs-task2 gets locked，it will directly return success >> even >> + * when the tmc-etr is not enabled at this moment. Ultimately, >> + * sysfs-task1 will still successfully enable tmc-etr. >> + * This is a transient state and does not cause an anomaly. >> + */ >> + coresight_set_mode(csdev, mode); >> + } >> + >> switch (mode) { >> case CS_MODE_SYSFS: >> - return tmc_enable_etr_sink_sysfs(csdev); >> + rc = tmc_enable_etr_sink_sysfs(csdev); >> + break; >> case CS_MODE_PERF: >> - return tmc_enable_etr_sink_perf(csdev, data); >> + rc = tmc_enable_etr_sink_perf(csdev, data); >> + break; >> default: >> - return -EINVAL; >> + rc = -EINVAL; >> } >> + >> + scoped_guard(spinlock_irqsave, &drvdata->spinlock) { >> + if (rc && old_mode != mode) >> + coresight_set_mode(csdev, old_mode); >> + } >> + >> + return rc; >> } >> static int tmc_disable_etr_sink(struct coresight_device *csdev) > > . >

2 weeks, 6 days

[PATCH v1 0/9] coresight: Fix and improve clock usage

by Leo Yan

This series fixes and improves clock usage in the Arm CoreSight drivers. Based on the DT binding documents, the trace clock (atclk) is defined in some CoreSight modules, but the corresponding drivers to support it are absent. In most cases, the issue is hidden because atclk is shared by multiple CoreSight modules and is enabled anyway by other drivers. The first three patches address this issue. The programming clock (pclk) management in CoreSight drivers does not use the devm_XXX() variant APIs, so the drivers needs to manually disable and release clocks for errors and for normal module exit. However, the drivers miss to disable clocks during module exit. Another issue is pclk might be enabled twice in init phase - once by AMBA bus driver, and again by CoreSight drivers. Patches 04 and 05 fix pclk issues. The atclk may also not be disabled in CoreSight drivers during module exit. This is fixed in patch 06. Patches 07 to 09 refactor the clock related code. Patch 07 makes the clock enabling sequence consistent. Patch 08 removes redundant condition checks and adds error handling in runtime PM. Patch 09 consolidats the clock initialization into a central place. This series is verified on Arm64 Hikey960 platform. Leo Yan (9): coresight: tmc: Support atclk coresight: catu: Support atclk coresight: etm4x: Support atclk coresight: Disable programming clock properly coresight: Avoid enable programming clock duplicately coresight: Disable trace bus clock properly coresight: Make clock sequence consistent coresight: Refactor runtime PM coresight: Consolidate clock enabling drivers/hwtracing/coresight/coresight-catu.c | 53 ++++++++++++++++----------------- drivers/hwtracing/coresight/coresight-catu.h | 1 + drivers/hwtracing/coresight/coresight-cpu-debug.c | 41 +++++++++----------------- drivers/hwtracing/coresight/coresight-ctcu-core.c | 24 +++++---------- drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++-------- drivers/hwtracing/coresight/coresight-etm3x-core.c | 17 ++++------- drivers/hwtracing/coresight/coresight-etm4x-core.c | 32 ++++++++++---------- drivers/hwtracing/coresight/coresight-etm4x.h | 4 ++- drivers/hwtracing/coresight/coresight-funnel.c | 66 +++++++++++++++--------------------------- drivers/hwtracing/coresight/coresight-replicator.c | 63 ++++++++++++++-------------------------- drivers/hwtracing/coresight/coresight-stm.c | 34 +++++++++------------- drivers/hwtracing/coresight/coresight-tmc-core.c | 48 +++++++++++++++--------------- drivers/hwtracing/coresight/coresight-tmc.h | 2 ++ drivers/hwtracing/coresight/coresight-tpiu.c | 36 ++++++++++------------- include/linux/coresight.h | 47 ++++++++++++++++++------------ 15 files changed, 206 insertions(+), 280 deletions(-) -- 2.34.1

2 weeks, 6 days

[PATCH 0/4] Coresight TMC-ETR some bugfixes and cleanups

by Junhao He

This patchset builds upon Yicong's previous patches [1]. Introducing fix two race issues found by using TMC-ETR and CATU, Two cleanups found when debugging the issues. [1] https://lore.kernel.org/linux-arm-kernel/20241202092419.11777-1-yangyicong@… Junhao He (2): coresight: catu: add locking to catu enable/disable functions coresight: tmc: refactor the tmc-etr mode setting Yicong Yang (2): coresight: tmc: Add missing doc of tmc_drvdata::reading coresight: tmc-etr: Decouple the perf buffer allocation from sysfs mode drivers/hwtracing/coresight/coresight-catu.c | 6 + drivers/hwtracing/coresight/coresight-catu.h | 1 + .../hwtracing/coresight/coresight-tmc-etr.c | 106 +++++++++--------- drivers/hwtracing/coresight/coresight-tmc.h | 1 + 4 files changed, 64 insertions(+), 50 deletions(-) -- 2.33.0

3 weeks, 3 days

Re: [PATCH v3 1/2] coresight: catu: Introduce refcount and spinlock for enabling/disabling

by James Clark

On 08/04/2025 8:59 pm, Yabin Cui wrote: > When tracing ETM data on multiple CPUs concurrently via the > perf interface, the CATU device is shared across different CPU > paths. This can lead to race conditions when multiple CPUs attempt > to enable or disable the CATU device simultaneously. > > To address these race conditions, this patch introduces the > following changes: > > 1. The enable and disable operations for the CATU device are not > reentrant. Therefore, a spinlock is added to ensure that only > one CPU can enable or disable a given CATU device at any point > in time. > > 2. A reference counter is used to manage the enable/disable state > of the CATU device. The device is enabled when the first CPU > requires it and is only disabled when the last CPU finishes > using it. This ensures the device remains active as long as at > least one CPU needs it. > > Signed-off-by: Yabin Cui <yabinc(a)google.com> > --- > drivers/hwtracing/coresight/coresight-catu.c | 25 +++++++++++++------- > drivers/hwtracing/coresight/coresight-catu.h | 1 + > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c > index fa170c966bc3..30b78b2f8adb 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -458,12 +458,17 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode, > static int catu_enable(struct coresight_device *csdev, enum cs_mode mode, > void *data) > { > - int rc; > + int rc = 0; > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); > + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); > Very minor nit only because you need to resend anyway, but there should be a newline between the variable definitions and the code. Not sure why checkpatch doesn't warn here. > - CS_UNLOCK(catu_drvdata->base); > - rc = catu_enable_hw(catu_drvdata, mode, data); > - CS_LOCK(catu_drvdata->base); > + if (csdev->refcnt == 0) { > + CS_UNLOCK(catu_drvdata->base); > + rc = catu_enable_hw(catu_drvdata, mode, data); > + CS_LOCK(catu_drvdata->base); > + } > + if (!rc) > + csdev->refcnt++; > return rc; > } > > @@ -486,12 +491,15 @@ static int catu_disable_hw(struct catu_drvdata *drvdata) > > static int catu_disable(struct coresight_device *csdev, void *__unused) > { > - int rc; > + int rc = 0; > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev); > + guard(raw_spinlock_irqsave)(&catu_drvdata->spinlock); > > - CS_UNLOCK(catu_drvdata->base); > - rc = catu_disable_hw(catu_drvdata); > - CS_LOCK(catu_drvdata->base); > + if (--csdev->refcnt == 0) { Hopefully this never underflows if disable is called again after a failed enable. We could add a WARN_ON() but I think this is a general case and not specific to these patches so is probably better to do later as separate change. Reviewed-by: James Clark <james.clark(a)linaro.org>

3 weeks, 6 days

Re: [PATCH v1 1/1] coresight: cti: Replace inclusion by struct fwnode_handle forward declaration

by Suzuki K Poulose

On 14/04/2025 11:06, Andy Shevchenko wrote: > On Mon, Mar 31, 2025 at 02:40:51PM +0100, James Clark wrote: >> On 31/03/2025 8:14 am, Andy Shevchenko wrote: > >> Reviewed-by: James Clark <james.clark(a)linaro.org> > > Thank you! Can it be applied now? > I will queue this via coresight tree, for v6.16 Cheers Suzuki

4 weeks

Re: [PATCH v2 0/5] coresight: ctcu: Enable byte-cntr function for TMC ETR

by Mike Leach

Hi, I can see that this patchset has fixed some of the issues raised from v1 - using the existing file handle for read, and stopping the ETR to read the RWP. However the fundamental problem that it is still attempting to read memory without stopping the ETR has not been addressed. As mentioned in mine and Suzuki's comments for v1, this means:- 1) you cannot guarantee that the buffer has not wrapped when reading data back, which will corrupt the trace decode process. 2) The DMA buffers are not being synchronized, so the PE could be reading stale data rather than the new data written by the ETR. Regards Mike On Thu, 10 Apr 2025 at 02:33, Jie Gan <jie.gan(a)oss.qualcomm.com> wrote: > > The byte-cntr function provided by the CTCU device is used to transfer data > from the ETR buffer to the userspace. An interrupt is tiggered if the data > size exceeds the threshold set in the BYTECNTRVAL register. The interrupt > handler counts the number of triggered interruptions and the read function > will read the data from the ETR buffer if the IRQ count is greater than 0. > The read work will be conducted ASAP after the byte-cntr is started. > Each successful read process will decrement the IRQ count by 1. > > The byte cntr function will start when the device node is opened for reading, > and the IRQ count will reset when the byte cntr function has stopped. When > the file node is opened, the w_offset of the ETR buffer will be read and > stored in byte_cntr_data, serving as the original r_offset (indicating > where reading starts) for the byte counter function. > > The work queue for the read operation will wake up once when ETR is stopped, > ensuring that the remaining data in the ETR buffer has been flushed based on > the w_offset read at the time of stopping. > > The byte-cntr read work has integrated with the file node tmc_etr, e.g. > /dev/tmc_etr0 > /dev/tmc_etr1 > > There are two scenarios for the ETR file nodes with byte-cntr function: > 1. BYTECNTRVAL register has configured -> byte-cntr read > 2. BYTECNTRVAL register is disabled -> original behavior, flush the etr_buf > > We still can flush the etr buffer once after the byte-cntr function has > triggered. > 1. Enable byte-cntr > 2. Byte-cntr read > 3. Disable byte-cntr > 4. Flush etr buffer > > Since the ETR operates in circular buffer mode, we cannot fully guarantee > that no overwrites occur when the byte-cntr read function reads the data. > The read function will read the data ASAP when the interrupt is > triggered and we should not configure a threshold greater than the > buffer size of the ETR buffer. > > The following shell commands write threshold to BYTECNTRVAL registers. > > Only enable byte-cntr for ETR0: > echo 0x10000 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > Enable byte-cntr for both ETR0 and ETR1(support both hex and decimal values): > echo 0x10000 4096 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > Setting the BYTECNTRVAL registers to 0 disables the byte-cntr function. > Disable byte-cntr for ETR0: > echo 0 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > Disable byte-cntr for both ETR0 and ETR1: > echo 0 0 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > > There is a minimum threshold to prevent generating too many interrupts. > The minimum threshold is 4096 bytes. The write process will fail if user try > to set the BYTECNTRVAL registers to a value less than 4096 bytes(except > for 0). > > Way to enable and start byte-cntr for ETR0: > echo 0x10000 > /sys/devices/platform/soc(a)0/4001000.ctcu/ctcu0/byte_cntr_val > echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink > echo 1 > /sys/bus/coresight/devices/etm0/enable_source > cat /dev/tmc_etr0 > > Testing case has conducted for the byte-cntr read work: > 1. Setting the buffer_size of the ETR as large as possile, here is for ETR0 > echo 0x1000000 > /sys/bus/coresight/devices/tmc_etr0/buffer_size > 2. Setting the threshold for the ETR0 to 0x10000 > echo 0x10000 > /sys/bus/coresight/devices/ctcu0/byte_cntr_val > 3. Enable ETR0 > echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink > 4. Enable ETM0 as source and enable byte-cntr to read data > echo 1 > /sys/bus/coresight/devices/etm0/enable_source; > cat /dev/tmc_etr0 > /tmp/file_byte_cntr.bin & > 5. Disable ETM0 > echo 0 > /sys/bus/coresight/devices/etm0/enable_source > 6. Disable byte-cntr and flush the etr buffer > echo 0 > /sys/bus/coresight/devices/ctcu0/byte_cntr_val; > cat /dev/tmc_etr0 > /tmp/file_etr0.bin > ls -l /tmp > > -rw-r--r-- 1 root root 12628960 Apr 28 17:44 file_byte_cntr.bin > -rw-r--r-- 1 root root 12669296 Apr 28 17:45 file_etr0.bin > > 7. Deal with the file_etr0.bin with following command: > dd if=/tmp/file_etr0.bin of=/tmp/file_etr0_aligned.bin bs=1 > count=12628960 skip=40336 > ls -l /tmp > > -rw-r--r-- 1 root root 12628960 Apr 28 17:44 file_byte_cntr.bin > -rw-r--r-- 1 root root 12669296 Apr 28 17:45 file_etr0.bin > -rw-r--r-- 1 root root 12628960 Apr 28 17:49 file_etr0_aligned.bin > > 8. Compared file_byte_cntr.bin with file_etr0_aligned.bin and identified > they are competely same. > diff file_byte_cntr.bin file_etr0_aligned.bin > > ======================= > Changes in V2: > 1. Removed the independent file node /dev/byte_cntr. > 2. Integrated the byte-cntr's file operations with current ETR file > node. > 3. Optimized the driver code of the CTCU that associated with byte-cntr. > 4. Add kernel document for the export API tmc_etr_get_rwp_offset. > 5. Optimized the way to read the rwp_offset according to Mike's > suggestion. > 6. Removed the dependency of the dts patch. > Link to V1 - https://lore.kernel.org/all/20250310090407.2069489-1-quic_jiegan@quicinc.co… > > Jie Gan (5): > coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer > dt-bindings: arm: Add an interrupt property for Coresight CTCU > coresight: ctcu: Enable byte-cntr for TMC ETR devices > coresight: tmc: add functions for byte-cntr operation > arm64: dts: qcom: sa8775p: Add interrupts to CTCU device > > .../bindings/arm/qcom,coresight-ctcu.yaml | 17 ++ > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 5 + > drivers/hwtracing/coresight/Makefile | 2 +- > .../coresight/coresight-ctcu-byte-cntr.c | 119 ++++++++++++ > .../hwtracing/coresight/coresight-ctcu-core.c | 88 ++++++++- > drivers/hwtracing/coresight/coresight-ctcu.h | 49 ++++- > .../hwtracing/coresight/coresight-tmc-core.c | 29 ++- > .../hwtracing/coresight/coresight-tmc-etr.c | 175 ++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tmc.h | 10 +- > 9 files changed, 483 insertions(+), 11 deletions(-) > create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c > > -- > 2.34.1 > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

1 month

Re: [PATCH v3 4/7] coresight-tgu: Add TGU decode support

by Mike Leach

Hi, On Thu, 10 Apr 2025 at 08:46, songchai <quic_songchai(a)quicinc.com> wrote: > > > On 3/7/2025 6:58 PM, Mike Leach wrote: > > Hi, > > On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai(a)quicinc.com> wrote: > > From: Songwei Chai <quic_songchai(a)quicinc.com> > > Decoding is when all the potential pieces for creating a trigger > are brought together for a given step. Example - there may be a > counter keeping track of some occurrences and a priority-group that > is being used to detect a pattern on the sense inputs. These 2 > inputs to condition_decode must be programmed, for a given step, > to establish the condition for the trigger, or movement to another > step. > > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com> > Signed-off-by: songchai <quic_songchai(a)quicinc.com> > --- > .../testing/sysfs-bus-coresight-devices-tgu | 7 ++ > drivers/hwtracing/coresight/coresight-tgu.c | 113 ++++++++++++++++-- > drivers/hwtracing/coresight/coresight-tgu.h | 29 ++++- > 3 files changed, 136 insertions(+), 13 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > index af7332153833..dd6cc1184d52 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > @@ -14,3 +14,10 @@ KernelVersion 6.15 > Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > Description: > (RW) Set/Get the sensed siganal with specific step and priority for TGU. > + > +What: /sys/bus/coresight/devices/<tgu-name>/step[0:7]_condition_decode/reg[0:3] > +Date: February 2025 > +KernelVersion 6.15 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > +Description: > + (RW) Set/Get the decode mode with specific step for TGU. > diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c > index f28761619ebe..5eebf5eecbbb 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.c > +++ b/drivers/hwtracing/coresight/coresight-tgu.c > @@ -22,9 +22,21 @@ static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index, > { > int ret = -EINVAL; > > - ret = operation_index * (drvdata->max_step) * > - (drvdata->max_reg) + step_index * (drvdata->max_reg) > - + reg_index; > + switch (operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + ret = operation_index * (drvdata->max_step) * > + (drvdata->max_reg) + step_index * (drvdata->max_reg) > + + reg_index; > + break; > + case TGU_CONDITION_DECODE: > + ret = step_index * (drvdata->max_condition_decode) + reg_index; > + break; > + default: > + break; > + } > > return ret; > } > @@ -36,10 +48,23 @@ static ssize_t tgu_dataset_show(struct device *dev, > struct tgu_attribute *tgu_attr = > container_of(attr, struct tgu_attribute, attr); > > - return sysfs_emit(buf, "0x%x\n", > - drvdata->value_table->priority[calculate_array_location( > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->priority[calculate_array_location( > > calculate_array_location() can return -EINVAL - you could be > referencing array value drvdata->value_table->priority[-EINVAL] here. > Test the return before referencing the array. > > Same for all following occurrences. > > Both tgu_dataset_store/show and calculate_array_location use switch cases to evaluate tgu_attr->operation_index. > > Therefore, when a case in tgu_dataset_store/show satisfies tgu_attr->operation_index, calculate_array_location > > will not return -EINVAL. This prevents the occurrence of -EINVAL within the array. If the function can never return -EINVAL, then do not use is at all in the function - it is unneeded. It does not matter that you believe that the current implementation can never produce an incorrect response - if you write a function that can return -EINVAL, this must be checked for - do not assume that future changes will retain the apparent "safety". Regards Mike > > drvdata, tgu_attr->step_index, > tgu_attr->operation_index, tgu_attr->reg_num)]); > + case TGU_CONDITION_DECODE: > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->condition_decode[calculate_array_location( > + drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)]); > + > > missing default - did this code compile without warnings? > > Will correct it in next version. > > + } > + return -EINVAL; > > } > > @@ -58,11 +83,25 @@ static ssize_t tgu_dataset_store(struct device *dev, > return ret; > > guard(spinlock)(&tgu_drvdata->spinlock); > - tgu_drvdata->value_table->priority[calculate_array_location( > - tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > - tgu_attr->reg_num)] = val; > - ret = size; > - > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + tgu_drvdata->value_table->priority[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + break; > + case TGU_CONDITION_DECODE: > + tgu_drvdata->value_table->condition_decode[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + break; > + default: > + break; > + } > return ret; > } > > @@ -79,8 +118,23 @@ static umode_t tgu_node_visible(struct kobject *kobject, struct attribute *attr, > container_of(dev_attr, struct tgu_attribute, attr); > > if (tgu_attr->step_index < drvdata->max_step) { > - ret = (tgu_attr->reg_num < drvdata->max_reg) ? > - attr->mode : 0; > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + ret = (tgu_attr->reg_num < drvdata->max_reg) ? > + attr->mode : 0; > + break; > + case TGU_CONDITION_DECODE: > + ret = (tgu_attr->reg_num < > + drvdata->max_condition_decode) ? > + attr->mode : 0; > + break; > + default: > + break; > + } > + > return ret; > } > return SYSFS_GROUP_INVISIBLE; > @@ -103,6 +157,17 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata) > } > } > > + for (i = 0; i < drvdata->max_step; i++) { > + for (j = 0; j < drvdata->max_condition_decode; j++) { > + tgu_writel(drvdata, > + drvdata->value_table > + ->condition_decode[calculate_array_location( > + drvdata, i, > + TGU_CONDITION_DECODE, j)], > + CONDITION_DECODE_STEP(i, j)); > + } > + } > + > /* Enable TGU to program the triggers */ > tgu_writel(drvdata, 1, TGU_CONTROL); > CS_LOCK(drvdata->base); > @@ -245,6 +310,14 @@ static const struct attribute_group *tgu_attr_groups[] = { > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1), > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2), > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(0), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(1), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(2), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(3), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(4), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7), > NULL, > }; > > @@ -289,6 +362,13 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > if (ret) > return -EINVAL; > > + ret = of_property_read_u32(adev->dev.of_node, "tgu-conditions", > + &drvdata->max_condition); > + if (ret) > + return -EINVAL; > + > + drvdata->max_condition_decode = drvdata->max_condition; > + > drvdata->value_table = > devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL); > if (!drvdata->value_table) > @@ -303,6 +383,15 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > if (!drvdata->value_table->priority) > return -ENOMEM; > > + drvdata->value_table->condition_decode = devm_kzalloc( > + dev, > + drvdata->max_condition_decode * drvdata->max_step * > + sizeof(*(drvdata->value_table->condition_decode)), > + GFP_KERNEL); > + > + if (!drvdata->value_table->condition_decode) > + return -ENOMEM; > + > drvdata->enable = false; > desc.type = CORESIGHT_DEV_TYPE_HELPER; > desc.pdata = adev->dev.platform_data; > diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h > index 6e5d465117df..c2a9ce38b44f 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.h > +++ b/drivers/hwtracing/coresight/coresight-tgu.h > @@ -46,6 +46,9 @@ > #define PRIORITY_REG_STEP(step, priority, reg)\ > (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step) > > +#define CONDITION_DECODE_STEP(step, decode) \ > + (0x0050 + 0x4 * decode + 0x1D8 * step) > + > > use #define constants with explanations of what they are rather than > arbitrary magic numbers. > > Done. > > #define tgu_dataset_ro(name, step_index, type, reg_num) \ > (&((struct tgu_attribute[]){ { \ > __ATTR(name, 0444, tgu_dataset_show, NULL), \ > @@ -66,6 +69,9 @@ > tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \ > reg_num) > > +#define STEP_DECODE(step_index, reg_num) \ > + tgu_dataset_rw(reg##reg_num, step_index, TGU_CONDITION_DECODE, reg_num) > + > #define STEP_PRIORITY_LIST(step_index, priority) \ > {STEP_PRIORITY(step_index, 0, priority), \ > STEP_PRIORITY(step_index, 1, priority), \ > @@ -88,6 +94,14 @@ > NULL \ > } > > +#define STEP_DECODE_LIST(n) \ > + {STEP_DECODE(n, 0), \ > + STEP_DECODE(n, 1), \ > + STEP_DECODE(n, 2), \ > + STEP_DECODE(n, 3), \ > + NULL \ > + } > + > #define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\ > (&(const struct attribute_group){\ > .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, priority),\ > @@ -95,11 +109,19 @@ > .name = "step" #step "_priority" #priority \ > }) > > +#define CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(step)\ > + (&(const struct attribute_group){\ > + .attrs = (struct attribute*[])STEP_DECODE_LIST(step),\ > + .is_visible = tgu_node_visible,\ > + .name = "step" #step "_condition_decode" \ > + }) > + > enum operation_index { > TGU_PRIORITY0, > TGU_PRIORITY1, > TGU_PRIORITY2, > - TGU_PRIORITY3 > + TGU_PRIORITY3, > + TGU_CONDITION_DECODE > > }; > > @@ -115,6 +137,7 @@ struct tgu_attribute { > > struct value_table { > unsigned int *priority; > + unsigned int *condition_decode; > }; > > /** > @@ -127,6 +150,8 @@ struct value_table { > * @value_table: Store given value based on relevant parameters. > * @max_reg: Maximum number of registers > * @max_step: Maximum step size > + * @max_condition: Maximum number of condition > + * @max_condition_decode: Maximum number of condition_decode > * > * This structure defines the data associated with a TGU device, including its base > * address, device pointers, clock, spinlock for synchronization, trigger data pointers, > @@ -141,6 +166,8 @@ struct tgu_drvdata { > struct value_table *value_table; > int max_reg; > int max_step; > + int max_condition; > + int max_condition_decode; > }; > > #endif > > Regards > > Mike > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

1 month

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight April 2025