Re: [PATCH 1/1] coresight: tmc-etr: Sync the trace buffer for the device

[Min Chen <chenmin83@gmail.com>]

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.