On 2025/6/20 19:53, Jonathan Cameron wrote:
> On Fri, 20 Jun 2025 15:54:10 +0800
> Junhao He <hejunhao3(a)huawei.com> wrote:
>
>> From: Yicong Yang <yangyicong(a)hisilicon.com>
>>
>> tmc_drvdata::reading is used to indicate whether a reading process
>> is performed through /dev/xyz.tmc. Document it.
>>
>> Reviewed-by: James Clark <james.clark(a)linaro.org>
>> Signed-off-by: Yicong Yang <yangyicong(a)hisilicon.com>
>> Signed-off-by: Junhao He <hejunhao3(a)huawei.com>
>> ---
>> drivers/hwtracing/coresight/coresight-tmc.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
>> index 6541a27a018e..3ca0d40c580d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>> @@ -220,6 +220,7 @@ struct tmc_resrv_buf {
>> * @pid: Process ID of the process that owns the session that is using
>> * this component. For example this would be the pid of the Perf
>> * process.
>> + * @reading: buffer's in the reading through "/dev/xyz.tmc" entry
> Hi,
>
> Perhaps reword:
>
> "buffer is being read through "/dev/xyz.tmc" entry" or
> "buffer read in progress through "/dev/xyz.tmc" entry"
>
> I've not checked what this actually means - just looking at what you have here.
Will update this. Thanks
Best regards,
Junhao.
>> * @stop_on_flush: Stop on flush trigger user configuration.
>> * @buf: Snapshot of the trace data for ETF/ETB.
>> * @etr_buf: details of buffer used in TMC-ETR
> .
>
On 2025/6/20 20:45, Jonathan Cameron wrote:
> On Fri, 20 Jun 2025 15:54:11 +0800
> Junhao He <hejunhao3(a)huawei.com> 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)
>>
>> A race condition is introduced here, perf always prioritizes execution, and
>> warnings can lead to concerns about potential hidden bugs, such as getting
>> out of sync.
>>
>> To fix this, configure the tmc-etr mode before invoking enable_etr_perf()
>> or enable_etr_sysfs(), explicitly check if the tmc-etr sink is already
>> enabled in a different mode, and simplily the setup and checks for "mode".
>> To prevent race conditions between mode transitions.
>>
>> 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 | 73 +++++++++++--------
>> 1 file changed, 43 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index b07fcdb3fe1a..252a57a8e94e 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -1263,11 +1263,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
>> raw_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.
>> @@ -1278,7 +1273,6 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
>> drvdata->sysfs_buf = new_buf;
>> }
>>
>> -out:
>> raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
>>
>> /* Free memory outside the spinlock if need be */
>> @@ -1289,7 +1283,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);
>> @@ -1299,23 +1293,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>>
>> raw_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:
>> raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
>>
>> if (!ret)
>> @@ -1735,11 +1716,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);
>>
>> raw_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;
>> @@ -1768,7 +1744,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++;
>> }
>> @@ -1781,14 +1756,52 @@ 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 = -EINVAL;
>> +
>> + 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. */
>> + if (old_mode == CS_MODE_SYSFS) {
> This seems odd. You are incrementing the reference count when potentially changing
> away from CS_MODE_SYSFS?
> Is this meant to only occur if old_mode == mode && old_mode == CS_MODE_SYSFS?
Yes, old_mode == CS_MODE_SYSFS means tmc_etr is occupied by sysfs, and
old_mode == mode, so can add reference counting directly.
When tmc_etr is idle its mode is CS_MODE_DISABLED.
> In the code prior to this patch this bit only ran in tmc_enable_etr_sink_sysfs()
> which was only called based on the mode being configured (mode here I think) being
> sysfs. That no longer looks to be the case.
before the patch, first check (mode == CS_MODE_SYSFS), then check
(coresight_get_mode(csdev) == CS_MODE_SYSFS),
and finally csdev->refcnt++. This is the same as the current functionality.
> Maybe I'm missing something as the flows around this are complex.
>
>
>> + csdev->refcnt++;
>> + return 0;
>> + }
>> +
>> + /*
>> + * minor note: In sysFS mode, the task1 get locked first, it setup
>> + * etr mode to SYSFS. Then task2 get locked,it will directly return
>> + * success even when the tmc-etr is not enabled at this moment.
>> + * Ultimately, 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;
>> }
>> +
>> + if (rc && old_mode != mode) {
>> + scoped_guard(spinlock_irqsave, &drvdata->spinlock) {
> Might be a local style matching thing but if not the scope is tight anyway
> so could use the unscoped version.
>
> guard(spinlock_irqsave)(&drvdata->spinlock);
> coresight_set_mode(csdev, old_mode);
Sure, Will fix in next version.
>> + coresight_set_mode(csdev, old_mode);
>> + }
>> + }
>> +
>> + return rc;
>> }
>>
>> static int tmc_disable_etr_sink(struct coresight_device *csdev)
> .
>
Ensure that etm_perf_add_symlink_sink() is only called for devices
that implement the alloc_buffer operation. This prevents invalid
symlink creation for dummy sinks that do not implement alloc_buffer.
Without this check, perf may attempt to use a dummy sink that lacks
alloc_buffer operationsu to initialise perf's ring buffer, leading
to runtime failures.
Fixes: 9d3ba0b6c0569 ("Coresight: Add coresight dummy driver")
Signed-off-by: Yuanfang Zhang <quic_yuanfang(a)quicinc.com>
---
drivers/hwtracing/coresight/coresight-core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fa758cc21827552a5c97b6bdd05d22dec4994b22..fddf04c5ee46eb4d559416296f7e85ce6c5689fa 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1374,8 +1374,9 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
goto out_unlock;
}
- if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
- csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
+ if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+ csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
+ sink_ops(csdev)->alloc_buffer) {
ret = etm_perf_add_symlink_sink(csdev);
if (ret) {
---
base-commit: 408c97c4a5e0b634dcd15bf8b8808b382e888164
change-id: 20250630-etm_perf_sink-ee0fcffc6820
Best regards,
--
Yuanfang Zhang <quic_yuanfang(a)quicinc.com>
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 support is absent. In most cases, the issue
is hidden because the atclk clock is shared by multiple CoreSight
modules and the clock 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, 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. The atclk may also
not be disabled in CoreSight drivers during module exit. By using devm
APIs, patches 04 and 05 fix clock disabling issues.
Another issue is pclk might be enabled twice in init phase - once by
AMBA bus driver, and again by CoreSight drivers. This is fixed in
patch 06.
Patches 07 to 10 refactor the clock related code. Patch 07 consolidates
the clock initialization into a central place. Patch 08 polishes driver
data allocation. Patch 09 makes the clock enabling sequence consistent.
Patch 09 removes redundant condition checks and adds error handling in
runtime PM.
This series has been verified on Arm64 Hikey960 and Juno platforms.
Changes from v3:
- Separated patch 07 into two patches, one is for clock consolidation
and another is for polishing driver data allocation (Anshuman).
Changes from v2:
- Updated subjects for patches 04 and 05 (Anshuman).
- Refined condition checking "if (dev_is_amba(dev))" in patch 07
(Anshuman).
Changes from v1:
- Moved the coresight_get_enable_clocks() function into CoreSight core
layer (James).
- Added comments for clock naming "apb_pclk" and "apb" (James).
- Re-ordered patches for easier understanding (Anshuman).
- Minor improvement for commit log in patch 01 (Anshuman).
Signed-off-by: Leo Yan <leo.yan(a)arm.com>
---
Leo Yan (10):
coresight: tmc: Support atclk
coresight: catu: Support atclk
coresight: etm4x: Support atclk
coresight: Appropriately disable programming clocks
coresight: Appropriately disable trace bus clocks
coresight: Avoid enable programming clock duplicately
coresight: Consolidate clock enabling
coresight: Refactor driver data allocation
coresight: Make clock sequence consistent
coresight: Refactor runtime PM
drivers/hwtracing/coresight/coresight-catu.c | 53 ++++++++---------
drivers/hwtracing/coresight/coresight-catu.h | 1 +
drivers/hwtracing/coresight/coresight-core.c | 46 +++++++++++++++
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 | 30 +---------
16 files changed, 226 insertions(+), 289 deletions(-)
---
base-commit: 67a993863163cb88b1b68974c31b0d84ece4293e
change-id: 20250627-arm_cs_fix_clock_v4-e24b1e1f8920
Best regards,
--
Leo Yan <leo.yan(a)arm.com>
Hi Levi,
On Fri, Jun 27, 2025 at 02:49:19PM +0100, Yeoreum Yun wrote:
> >
> > > @@ -789,6 +789,10 @@ static int __tmc_probe(struct device *dev, struct resource *res)
> > > struct coresight_desc desc = { 0 };
> > > struct coresight_dev_list *dev_list = NULL;
> > >
> > > + drvdata->atclk = devm_clk_get_optional_enabled(dev, "atclk");
> > > + if (IS_ERR(drvdata->atclk))
> > > + return PTR_ERR(drvdata->atclk);
> > > +
> > > ret = -ENOMEM;
> > >
> >
> > Just another quetion.
> >
> > If this function is called from tmc_platform_probe() and failed,
> > should it call the clk_put() for drvdata->pclk when it failed?
>
> Sorry, I missed the Patch #7.
No worries.
devm_clk_release() is a registered callback used by the device model
layer to release resources. The clock will be released in the flow:
devm_clk_release()
`> clk_put()
`> free_clk()
Thanks,
Leo
On 2025/6/18 11:16, Bjorn Andersson wrote:
> On Thu, Apr 24, 2025 at 04:58:52AM -0700, Mao Jinlong wrote:
>> qcom,qmi-id is required for remote etm driver to find the remote
>> subsystem connection. It is the instance id used by qmi API to
>> communicate with remote processor.
>>
>> Signed-off-by: Mao Jinlong <quic_jinlmao(a)quicinc.com>
>> ---
>> .../bindings/arm/qcom,coresight-remote-etm.yaml | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
>> index 4fd5752978cd..947fe33738a3 100644
>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-remote-etm.yaml
>> @@ -20,6 +20,13 @@ properties:
>> compatible:
>> const: qcom,coresight-remote-etm
>>
>> + qcom,qmi-id:
>
> Why isn't this "qcom,qmi-instance-id" if that's what it represents?
ok.
>
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + This id is used by qmi API to communicate with remote processor for
>> + enabling and disabling remote etm. Each processor has its unique instance
>> + id.
>
> DeviceTree describes the hardware and firmware interface, so don't
> describe properties in terms of what Linux will do with this value, but
> what it represents.
Sure. I will update it.
>
>> +
>> out-ports:
>> $ref: /schemas/graph.yaml#/properties/ports
>> additionalProperties: false
>> @@ -32,6 +39,7 @@ properties:
>> required:
>> - compatible
>> - out-ports
>> + - qcom,qmi-id
>
> How can this suddenly be required, did devices described by this binding
> up until this point not work?
Without this instance id, remote etm won't work.
>
> If this is the case, make sure to clearly describe this in the commit
> message.
>
> Regards,
> Bjorn
>
>>
>> additionalProperties: false
>>
>> @@ -40,6 +48,8 @@ examples:
>> etm {
>> compatible = "qcom,coresight-remote-etm";
>>
>> + qcom,qmi-id = <2>;
>> +
>> out-ports {
>> port {
>> modem_etm0_out_funnel_modem: endpoint {
>> --
>> 2.25.1
>>
On 23/06/2025 9:59 am, Keita Morisaki wrote:
>> We have the ETM driver performing the save/restore of ETM context during
>> a CPUidle. This is only done when the ETM/ETE is described to be loosing
>> context over PM operation. If this is not done (via DT), the driver
>> doesn't do anything. This could be problematic. Could you try adding:
>>
>> "arm,coresight-loses-context-with-cpu"
>>
>>
>> property to the ETE nodes and see if it makes a difference ?
>
> I tried this in our environment, and this worked well. The "arm,coresight-loses-context-with-cpu" property was what we needed.
> Thank you so much again for the swift response with the useful information!
>
> Best,
> Keita
Hi Keita,
Thanks for the report. We discussed internally and decided that it would
be better for the driver to always save the context by default, because
this mistake is easy to make. Saving when it doesn't need to be saved
doesn't do any harm, but not saving when it should be causes quite bad bugs.
So "arm,coresight-loses-context-with-cpu" will be ignored in the future
and we'll add a new flag like "arm,coresight-save-context" if anyone
wants the optimization of not saving.
Thanks
James