mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/1] coresight: tmc-etr: Sync the trace buffer for the device
@ 2026-09-15 13:05 NoNine
  2026-09-15 13:05 ` [PATCH 1/1] " NoNine
  0 siblings, 1 reply; 5+ messages in thread
From: NoNine @ 2026-09-15 13:05 UTC (permalink / raw)
  To: suzuki.poulose
  Cc: mike.leach, james.clark, alexander.shishkin, coresight,
	linux-arm-kernel, linux-kernel, Min Chen

From: Min Chen <min.chen@siengine.com>

The flat ETR trace buffer is allocated with dma_alloc_noncoherent(), which
reaches dma_alloc_pages().  That API zeroes the buffer with CPU stores and
requires the caller to sync the memory for the device before the device
writes into it.  The TMC driver only ever syncs for the CPU afterwards, so
a non-coherent sink is handed a buffer whose zero fill is still dirty in
cache.

On an AD1000 EVB this shows up as 64-byte-aligned runs of all-zero
formatter frames in the first window of a freshly allocated buffer
(1,100-2,400 runs in the leading ~4 MiB; later windows and reused buffers
are clean).  Writing a known pattern over the buffer before the arm and
cleaning it removes every run, which pins the loss to the buffer
hand-over.  The ETR node is not marked dma-coherent.

The patch adds a sync_for_device() buffer operation, implemented by the
flat buffer, and calls it from __tmc_etr_enable_hw() before the TMC is
enabled.  It is intentionally flat-only: the ETR_SG and CATU data pages
are allocated and immediately dma_map_page()d, so the map performs the
architecture's for-device maintenance for the first hand-over.  Their one
remaining corner case - a barrier packet written into a full buffer before
a live drain re-arms the same buffer - has no reproducer here and is left
out.

Tested on the AD1000 EVB: five first windows on freshly allocated buffers,
including the first capture of a boot, carry no all-zero-frame runs and
show the ordinary clean-stream profile (2.851% zero bytes), against
1,141-2,360 runs for the same measurement without the change.  The patch
was rebased onto the Coresight for-next/queue tip 5442d22da7db and builds
with arm64 defconfig (Image and modules, no warnings or errors).  The
hardware run used the vendor 6.6.87 tree; the rebase carries the same
change.

Min Chen (1):
  coresight: tmc-etr: Sync the trace buffer for the device

 .../hwtracing/coresight/coresight-tmc-etr.c   | 29 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
 2 files changed, 30 insertions(+)

-- 
2.34.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/1] coresight: tmc-etr: Sync the trace buffer for the device
  2026-09-15 13:05 [PATCH 0/1] coresight: tmc-etr: Sync the trace buffer for the device NoNine
@ 2026-09-15 13:05 ` NoNine
  2026-09-16  3:04   ` Jie Gan
  0 siblings, 1 reply; 5+ messages in thread
From: NoNine @ 2026-09-15 13:05 UTC (permalink / raw)
  To: suzuki.poulose
  Cc: mike.leach, james.clark, alexander.shishkin, coresight,
	linux-arm-kernel, linux-kernel, Min Chen

From: Min Chen <min.chen@siengine.com>

The flat ETR buffer comes from dma_alloc_noncoherent(), which zeroes it
with CPU stores.  The DMA API requires the caller to sync the buffer for
the device before the device writes into it, but the TMC driver only
ever syncs for the CPU afterwards.  On a non-coherent sink the zero fill
is therefore still dirty in cache when the ETR starts writing, and its
write-back lands on top of the trace data.

Add a sync_for_device() buffer operation and call it from
__tmc_etr_enable_hw() just before the TMC is enabled.  Only the flat
buffer implements it.  The ETR_SG and CATU data pages are synced by
dma_map_page() when they are allocated; their remaining corner case, a
barrier packet followed by a live-drain re-arm, is left for a separate
change.

Tested on an AD1000 EVB: five first windows on freshly allocated
buffers, including the first capture of a boot, all without the
previous all-zero-formatter-frame runs.

Signed-off-by: Min Chen <min.chen@siengine.com>
---
 .../hwtracing/coresight/coresight-tmc-etr.c   | 29 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 76a8cb2..bf1d6c6 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -689,10 +689,29 @@ static ssize_t tmc_etr_get_data_flat_buf(struct etr_buf *etr_buf,
 	return len;
 }
 
+/*
+ * tmc_etr_sync_flat_buf_for_device: Drop any CPU cache lines over the trace
+ * buffer before the ETR is allowed to write into it.  The buffer is allocated
+ * with dma_alloc_noncoherent(), which zeroes it with CPU stores, and the DMA
+ * API requires a sync for the device before the device writes into the
+ * memory.  Without it a non-coherent sink writes into memory while the zero
+ * fill is still dirty in cache, and the write-back lands on top of the trace
+ * data.
+ */
+static void tmc_etr_sync_flat_buf_for_device(struct etr_buf *etr_buf)
+{
+	struct etr_flat_buf *flat_buf = etr_buf->private;
+	struct device *real_dev = flat_buf->dev->parent;
+
+	dma_sync_single_for_device(real_dev, flat_buf->daddr, etr_buf->size,
+				   DMA_FROM_DEVICE);
+}
+
 static const struct etr_buf_operations etr_flat_buf_ops = {
 	.alloc = tmc_etr_alloc_flat_buf,
 	.free = tmc_etr_free_flat_buf,
 	.sync = tmc_etr_sync_flat_buf,
+	.sync_for_device = tmc_etr_sync_flat_buf_for_device,
 	.get_data = tmc_etr_get_data_flat_buf,
 };
 
@@ -1113,6 +1132,16 @@ static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
 	writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
 
 	writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
+
+	/*
+	 * Hand the buffer over in a state the device can write into: drop
+	 * any dirty CPU cache lines first, or they get written back over
+	 * the trace data the ETR produces.  Only the flat buffer needs
+	 * this; the ETR_SG and CATU data pages are synced by
+	 * dma_map_page() when they are allocated.
+	 */
+	if (etr_buf->ops->sync_for_device)
+		etr_buf->ops->sync_for_device(etr_buf);
 	tmc_enable_hw(drvdata);
 
 	CS_LOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 6541a27..3dd17da 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -277,6 +277,7 @@ struct etr_buf_operations {
 	int (*alloc)(struct tmc_drvdata *drvdata, struct etr_buf *etr_buf,
 		     int node, void **pages);
 	void (*sync)(struct etr_buf *etr_buf, u64 rrp, u64 rwp);
+	void (*sync_for_device)(struct etr_buf *etr_buf);
 	ssize_t (*get_data)(struct etr_buf *etr_buf, u64 offset, size_t len,
 			    char **bufpp);
 	void (*free)(struct etr_buf *etr_buf);
-- 
2.34.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] coresight: tmc-etr: Sync the trace buffer for the device
  2026-09-15 13:05 ` [PATCH 1/1] " NoNine
@ 2026-09-16  3:04   ` Jie Gan
  2026-09-16 14:11     ` Leo Yan
  0 siblings, 1 reply; 5+ messages in thread
From: Jie Gan @ 2026-09-16  3:04 UTC (permalink / raw)
  To: NoNine, suzuki.poulose
  Cc: mike.leach, james.clark, alexander.shishkin, coresight,
	linux-arm-kernel, linux-kernel, Min Chen

Hi,

On 9/15/2026 9:05 PM, NoNine wrote:
> From: Min Chen <min.chen@siengine.com>
> 
> The flat ETR buffer comes from dma_alloc_noncoherent(), which zeroes it
> with CPU stores.  The DMA API requires the caller to sync the buffer for
> the device before the device writes into it, but the TMC driver only
> ever syncs for the CPU afterwards.  On a non-coherent sink the zero fill
> is therefore still dirty in cache when the ETR starts writing, and its
> write-back lands on top of the trace data.
> 

Agree, without the sync, the dirty data may overwrites the trace data.

> Add a sync_for_device() buffer operation and call it from
> __tmc_etr_enable_hw() just before the TMC is enabled.  Only the flat
> buffer implements it.  The ETR_SG and CATU data pages are synced by
> dma_map_page() when they are allocated; their remaining corner case, a
> barrier packet followed by a live-drain re-arm, is left for a separate
> change.
> 
> Tested on an AD1000 EVB: five first windows on freshly allocated
> buffers, including the first capture of a boot, all without the
> previous all-zero-formatter-frame runs.
> 
> Signed-off-by: Min Chen <min.chen@siengine.com>
> ---
>   .../hwtracing/coresight/coresight-tmc-etr.c   | 29 +++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 76a8cb2..bf1d6c6 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -689,10 +689,29 @@ static ssize_t tmc_etr_get_data_flat_buf(struct etr_buf *etr_buf,
>   	return len;
>   }
>   
> +/*
> + * tmc_etr_sync_flat_buf_for_device: Drop any CPU cache lines over the trace
> + * buffer before the ETR is allowed to write into it.  The buffer is allocated
> + * with dma_alloc_noncoherent(), which zeroes it with CPU stores, and the DMA
> + * API requires a sync for the device before the device writes into the
> + * memory.  Without it a non-coherent sink writes into memory while the zero
> + * fill is still dirty in cache, and the write-back lands on top of the trace
> + * data.
> + */
> +static void tmc_etr_sync_flat_buf_for_device(struct etr_buf *etr_buf)
> +{
> +	struct etr_flat_buf *flat_buf = etr_buf->private;
> +	struct device *real_dev = flat_buf->dev->parent;
> +
> +	dma_sync_single_for_device(real_dev, flat_buf->daddr, etr_buf->size,
> +				   DMA_FROM_DEVICE);
> +}
> +
>   static const struct etr_buf_operations etr_flat_buf_ops = {
>   	.alloc = tmc_etr_alloc_flat_buf,
>   	.free = tmc_etr_free_flat_buf,
>   	.sync = tmc_etr_sync_flat_buf,
> +	.sync_for_device = tmc_etr_sync_flat_buf_for_device,
>   	.get_data = tmc_etr_get_data_flat_buf,
>   };
>   
> @@ -1113,6 +1132,16 @@ static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
>   	writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
>   
>   	writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
> +
> +	/*
> +	 * Hand the buffer over in a state the device can write into: drop
> +	 * any dirty CPU cache lines first, or they get written back over
> +	 * the trace data the ETR produces.  Only the flat buffer needs
> +	 * this; the ETR_SG and CATU data pages are synced by
> +	 * dma_map_page() when they are allocated.
> +	 */
> +	if (etr_buf->ops->sync_for_device)
> +		etr_buf->ops->sync_for_device(etr_buf);

Can we add a condition to perform the synchronization only when flat_buf 
mode is enabled, instead of introducing a new operation that is required 
only for flat_buf mode?

if (etr_buf->mode == ETR_MODE_FLAT)
	tmc_etr_sync_flat_buf_for_device(etr_buf);

Thanks,
Jie

>   	tmc_enable_hw(drvdata);
>   
>   	CS_LOCK(drvdata->base);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 6541a27..3dd17da 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -277,6 +277,7 @@ struct etr_buf_operations {
>   	int (*alloc)(struct tmc_drvdata *drvdata, struct etr_buf *etr_buf,
>   		     int node, void **pages);
>   	void (*sync)(struct etr_buf *etr_buf, u64 rrp, u64 rwp);
> +	void (*sync_for_device)(struct etr_buf *etr_buf);
>   	ssize_t (*get_data)(struct etr_buf *etr_buf, u64 offset, size_t len,
>   			    char **bufpp);
>   	void (*free)(struct etr_buf *etr_buf);


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] coresight: tmc-etr: Sync the trace buffer for the device
  2026-09-16  3:04   ` Jie Gan
@ 2026-09-16 14:11     ` Leo Yan
  2026-09-19  7:47       ` Min Chen
  0 siblings, 1 reply; 5+ messages in thread
From: Leo Yan @ 2026-09-16 14:11 UTC (permalink / raw)
  To: Jie Gan
  Cc: NoNine, suzuki.poulose, mike.leach, james.clark,
	alexander.shishkin, coresight, linux-arm-kernel, linux-kernel,
	Min Chen

On Wed, Sep 16, 2026 at 11:04:20AM +0800, Jie Gan wrote:

[...]

> > From: Min Chen <min.chen@siengine.com>
> > 
> > The flat ETR buffer comes from dma_alloc_noncoherent(), which zeroes it
> > with CPU stores.  The DMA API requires the caller to sync the buffer for
> > the device before the device writes into it, but the TMC driver only
> > ever syncs for the CPU afterwards.  On a non-coherent sink the zero fill
> > is therefore still dirty in cache when the ETR starts writing, and its
> > write-back lands on top of the trace data.

Good catch! I'm curious how you observed the dirty cache lines
overwriting trace data in DDR and causing corruption.

> Agree, without the sync, the dirty data may overwrites the trace data.

> > Add a sync_for_device() buffer operation and call it from
> > __tmc_etr_enable_hw() just before the TMC is enabled.

I don't think __tmc_etr_enable_hw() is the best place for the sync, as
it can be called frequently when an event is enabled, e.g. when a task
is scheduled in or migrated between CPUs. We should be able to sync
once after dma_alloc_noncoherent() instead.

The issue is not limited to buffer init. The driver also injects barrier
packets into the bounce buffer, which can race with the sink. Even
worse, the barrier packet write may collide with trace data when they
share a cache line.

I think we should consider writing barrier packets directly into the
AUX buffer. This would avoid stale cache data from barrier packet writes
and simplify the flow without additional sync operations.

Would you mind if I pick up this patch (keeping you as the author) and
add a second patch to address the barrier packet issue? That part may
need some several rounds refactoring so can have better shape, I think
it would be easier to consolidate the fixes on my side.

Thanks,
Leo

P.s. Please CC me on future CoreSight patches. If you're using the
mainline ./scripts/get_maintainer.pl, it should add me automatically.
I didn't receive this patch directly, which is why I'm replying to
Jie's email (also thanks Jie's review).

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] coresight: tmc-etr: Sync the trace buffer for the device
  2026-09-16 14:11     ` Leo Yan
@ 2026-09-19  7:47       ` Min Chen
  0 siblings, 0 replies; 5+ messages in thread
From: Min Chen @ 2026-09-19  7:47 UTC (permalink / raw)
  To: leo.yan
  Cc: alexander.shishkin, chenmin83, coresight, james.clark, jie.gan,
	linux-arm-kernel, linux-kernel, mike.leach, min.chen,
	suzuki.poulose

From: Min Chen <chenmin83@gmail.com>

Hi Leo,

> Good catch! I'm curious how you observed the dirty cache lines
> overwriting trace data in DDR and causing corruption.

The story is straightforward, I was testing the newly implemented CoreSight
source driver of AD1000 NPU subsystem and in the captured trace data the
starting part of a couple of MBs cannot be extracted by following the ARC
Trace spec.

Then parse the malformed data:
  Leading region - all-zero formatter frames, nothing else (first non-zero
  byte is at 0x180):

  00000000: 0000 0000 0000 0000 0000 0000 0000 0000
  00000010: 0000 0000 0000 0000 0000 0000 0000 0000
  00000020: 0000 0000 0000 0000 0000 0000 0000 0000
  00000030: 0000 0000 0000 0000 0000 0000 0000 0000

  zero frames interleaved into the fill; 5 of the 8 frames
  here are zero, then the fill resumes at 0x40080:

  00040030: 0000 0000 0000 0000 0000 0000 0000 0000
  00040040: 0000 0000 0000 0000 0000 0000 0000 0000
  00040050: 0000 0000 0000 0000 0000 0000 0000 0000
  00040060: 0000 0000 0000 0000 0000 0000 0000 0000
  00040070: 0000 0000 0000 0000 0000 0000 0000 0000
  00040080: feff fe36 f0ce c036 f0ce c036 f0ce c003
  00040090: 54ff feff 36f0 cec0 36f0 cec0 36f0 ce00
  000400a0: c054 fefe fe36 f0ce c036 f0ce c036 f006

  The same area in the unwrapped ATDATA stream (offset 0x40000), where
  the zeros appear as 60-byte runs bounded by truncated fill words
   - 36 f0 ce 00 on entry, 00 c0 54 ff fe ff on exit:

  0003fff0: 36f0 cec0 54ff feff 36f0 cec0 36f0 cec0
  00040000: 36f0 cec0 54ff feff ffff ffff 36f0 cec0
  00040010: 36f0 cec0 36f0 cec0 54ff feff 36f0 ce00
  00040020: 0000 0000 0000 0000 0000 0000 0000 0000
  ...
  00040050: 0000 0000 0000 00c0 54ff feff 36f0 cec0
  00040060: 36f0 cec0 36f0 cec0 54ff feff 36f0 cec0
  00040080: 36f0 cec0 54ff feff ffff ffff 36f0 cec0

  Note the quantization: every zero run in the plateau is a multiple
  of 60 ATDATA bytes (60, 120, 180, 240, ... - 6,251 runs at 60 B, 1,467 at
  120 B, decaying), i.e. 4, 8, 12 all-zero formatter frames, minimum 4.
  There is no run shorter than 4 frames and no other non-fill content
  between them.

The non-zero data is not random corruption, most of them are decodable as
ARC Trace message. And a further experiment confirms that the error mode is
overwritting not inserting, where the zeros are removed and trying to decode
the remaining stream still reports errors.

The next step is trying to figure out how the overwriting happens by writing
a fixed pattern 0x5a5aa5a5 to the ETR flat buffer before enabling it, and a
call to dma_sync_single_for_device() is added to make sure the patterns
are flushed to memory.

Then the issue cannot be reproduced any more, it is detected a cache sync
problem (dma_sync_single_for_device fixed the issue) without too much effect
since the debug change is small.

> > Agree, without the sync, the dirty data may overwrites the trace data.

> I don't think __tmc_etr_enable_hw() is the best place for the sync, as
> it can be called frequently when an event is enabled, e.g. when a task
> is scheduled in or migrated between CPUs. We should be able to sync
> once after dma_alloc_noncoherent() instead.

Agree, perform the sync just after the buffer/page allocation is reasonable
and this is what ETR_SG and CATU paths already do.

> The issue is not limited to buffer init. The driver also injects barrier
> packets into the bounce buffer, which can race with the sink. Even
> worse, the barrier packet write may collide with trace data when they
> share a cache line.

I think the timing of the barrier packet writing need further confirmation.
Is it only written to the buffer when the ETR is disabled?

> I think we should consider writing barrier packets directly into the
> AUX buffer. This would avoid stale cache data from barrier packet writes
> and simplify the flow without additional sync operations.

The AUX buffer only covers Perf path, and what I used is the
sysfs path:
  echo 0x10000000 > /sys/bus/coresight/devices/tmc_etr0/buffer_size
  echo 1          > /sys/bus/coresight/devices/tmc_etr0/enable_sink
  echo 1          > /sys/bus/coresight/devices/arct0/enable_source
  # Run the workload here
  echo 0          > /sys/bus/coresight/devices/arct0/enable_source
  echo 0          > /sys/bus/coresight/devices/tmc_etr0/enable_sink
  cat /dev/tmc_etr0 > arc-trace.bin

Need a solution for both paths.

> Would you mind if I pick up this patch (keeping you as the author) and
> add a second patch to address the barrier packet issue? That part may
> need some several rounds refactoring so can have better shape, I think
> it would be easier to consolidate the fixes on my side.

Please go ahead.

> Thanks,
> Leo

> P.s. Please CC me on future CoreSight patches. If you're using the
> mainline ./scripts/get_maintainer.pl, it should add me automatically.
> I didn't receive this patch directly, which is why I'm replying to
> Jie's email (also thanks Jie's review).

It's my fault, I did not get the list from the latest trunk.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-09-19  7:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-15 13:05 [PATCH 0/1] coresight: tmc-etr: Sync the trace buffer for the device NoNine
2026-09-15 13:05 ` [PATCH 1/1] " NoNine
2026-09-16  3:04   ` Jie Gan
2026-09-16 14:11     ` Leo Yan
2026-09-19  7:47       ` Min Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®