[PATCH 0/2] coresight: Add barrier packet when moving offset forward

[PATCH 0/2] coresight: Add barrier packet when moving offset forward

[Mathieu Poirier]

Hi Yabin,

When doing more tests on your patch that adjust the offset to fit the  
available space in the perf ring buffer[1], I noticed the decoder wasn't
able to decode the traces that had been collected.  The issue was observed
in CPU wide scenarios but I also suspect they would have showed up in
per-thread mode given the right conditions.

I traced the problem to the moving forward of the offset in the trace
buffer.  Doing so skips over the barrier packets originally inserted in
function tmc_sync_etr_buf(), which in turn prevents the decoder from
properly synchronising with the trace packets.

I fixed the condition by inserting barrier packets once the offset has been
moved forward, making sure that alignment rules are respected.

I'd be grateful if you could review and test my changes to make sure things
still work on your side.

Applies cleanly on the coresight next branch.

Best regards,
Mathieu 

[1]. https://lkml.org/lkml/2019/8/14/1336


Mathieu Poirier (2):
  coresight: tmc: Make memory width mask computation into a function
  coresight: tmc-etr: Add barrier packet when moving offset forward

 .../hwtracing/coresight/coresight-tmc-etf.c   | 23 +---------
 .../hwtracing/coresight/coresight-tmc-etr.c   | 43 ++++++++++++++-----
 drivers/hwtracing/coresight/coresight-tmc.c   | 28 ++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
 4 files changed, 64 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH 1/2] coresight: tmc: Make memory width mask computation into a function

[Mathieu Poirier]

Make the computation of a memory ask representing the width of the memory
bus into a function so that it can be re-used by the ETR driver.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../hwtracing/coresight/coresight-tmc-etf.c   | 23 ++-------------
 drivers/hwtracing/coresight/coresight-tmc.c   | 28 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
 3 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 23b7ff00af5c..807416b75ecc 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -479,30 +479,11 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
 	 * traces.
 	 */
 	if (!buf->snapshot && to_read > handle->size) {
-		u32 mask = 0;
-
-		/*
-		 * The value written to RRP must be byte-address aligned to
-		 * the width of the trace memory databus _and_ to a frame
-		 * boundary (16 byte), whichever is the biggest. For example,
-		 * for 32-bit, 64-bit and 128-bit wide trace memory, the four
-		 * LSBs must be 0s. For 256-bit wide trace memory, the five
-		 * LSBs must be 0s.
-		 */
-		switch (drvdata->memwidth) {
-		case TMC_MEM_INTF_WIDTH_32BITS:
-		case TMC_MEM_INTF_WIDTH_64BITS:
-		case TMC_MEM_INTF_WIDTH_128BITS:
-			mask = GENMASK(31, 4);
-			break;
-		case TMC_MEM_INTF_WIDTH_256BITS:
-			mask = GENMASK(31, 5);
-			break;
-		}
+		u32 mask = tmc_get_memwidth_mask(drvdata);
 
 		/*
 		 * Make sure the new size is aligned in accordance with the
-		 * requirement explained above.
+		 * requirement explained in function tmc_get_memwidth_mask().
 		 */
 		to_read = handle->size & mask;
 		/* Move the RAM read pointer up */
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 3055bf8e2236..1cf82fa58289 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -70,6 +70,34 @@ void tmc_disable_hw(struct tmc_drvdata *drvdata)
 	writel_relaxed(0x0, drvdata->base + TMC_CTL);
 }
 
+u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata)
+{
+	u32 mask = 0;
+
+	/*
+	 * When moving RRP or an offset address forward, the new values must
+	 * be byte-address aligned to the width of the trace memory databus
+	 * _and_ to a frame boundary (16 byte), whichever is the biggest. For
+	 * example, for 32-bit, 64-bit and 128-bit wide trace memory, the four
+	 * LSBs must be 0s. For 256-bit wide trace memory, the five LSBs must
+	 * be 0s.
+	 */
+	switch (drvdata->memwidth) {
+	case TMC_MEM_INTF_WIDTH_32BITS:
+	/* fallthrough */
+	case TMC_MEM_INTF_WIDTH_64BITS:
+	/* fallthrough */
+	case TMC_MEM_INTF_WIDTH_128BITS:
+		mask = GENMASK(31, 4);
+		break;
+	case TMC_MEM_INTF_WIDTH_256BITS:
+		mask = GENMASK(31, 5);
+		break;
+	}
+
+	return mask;
+}
+
 static int tmc_read_prepare(struct tmc_drvdata *drvdata)
 {
 	int ret = 0;
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 9dbcdf453e22..71de978575f3 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -255,6 +255,7 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
 void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
 void tmc_enable_hw(struct tmc_drvdata *drvdata);
 void tmc_disable_hw(struct tmc_drvdata *drvdata);
+u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
 
 /* ETB/ETF functions */
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata);
-- 
2.17.1

[PATCH 2/2] coresight: tmc-etr: Add barrier packet when moving offset forward

[Mathieu Poirier]

This patch adds barrier packets in the trace stream when the offset in the
data buffer needs to be moved forward.  Otherwise the decoder isn't aware
of the break in the stream and can't synchronise itself with the trace
data.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../hwtracing/coresight/coresight-tmc-etr.c   | 43 ++++++++++++++-----
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 4f000a03152e..0e4cd6ec5f28 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -946,10 +946,6 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
 	WARN_ON(!etr_buf->ops || !etr_buf->ops->sync);
 
 	etr_buf->ops->sync(etr_buf, rrp, rwp);
-
-	/* Insert barrier packets at the beginning, if there was an overflow */
-	if (etr_buf->full)
-		tmc_etr_buf_insert_barrier_packet(etr_buf, etr_buf->offset);
 }
 
 static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
@@ -1415,10 +1411,11 @@ static void tmc_free_etr_buffer(void *config)
  * buffer to the perf ring buffer.
  */
 static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
+				     unsigned long src_offset,
 				     unsigned long to_copy)
 {
 	long bytes;
-	long pg_idx, pg_offset, src_offset;
+	long pg_idx, pg_offset;
 	unsigned long head = etr_perf->head;
 	char **dst_pages, *src_buf;
 	struct etr_buf *etr_buf = etr_perf->etr_buf;
@@ -1427,7 +1424,6 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
 	pg_idx = head >> PAGE_SHIFT;
 	pg_offset = head & (PAGE_SIZE - 1);
 	dst_pages = (char **)etr_perf->pages;
-	src_offset = etr_buf->offset + etr_buf->len - to_copy;
 
 	while (to_copy > 0) {
 		/*
@@ -1475,7 +1471,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 		      void *config)
 {
 	bool lost = false;
-	unsigned long flags, size = 0;
+	unsigned long flags, offset, size = 0;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 	struct etr_perf_buffer *etr_perf = config;
 	struct etr_buf *etr_buf = etr_perf->etr_buf;
@@ -1503,11 +1499,39 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	size = etr_buf->len;
+	offset = etr_buf->offset;
+	lost |= etr_buf->full;
+
+	/*
+	 * The ETR buffer may be bigger than the space available in the
+	 * perf ring buffer (handle->size).  If so advance the offset so that we
+	 * get the latest trace data.  In snapshot mode none of that matters
+	 * since we are expected to clobber stale data in favour of the latest
+	 * traces.
+	 */
 	if (!etr_perf->snapshot && size > handle->size) {
-		size = handle->size;
+		u32 mask = tmc_get_memwidth_mask(drvdata);
+
+		/*
+		 * Make sure the new size is aligned in accordance with the
+		 * requirement explained in function tmc_get_memwidth_mask().
+		 */
+		size = handle->size & mask;
+		offset = etr_buf->offset + etr_buf->len - size;
+
+		if (offset >= etr_buf->size)
+			offset -= etr_buf->size;
 		lost = true;
 	}
-	tmc_etr_sync_perf_buffer(etr_perf, size);
+
+	/*
+	 * Insert barrier packets at the beginning, if there was an overflow
+	 * or if the offset had to be brought forward.
+	 */
+	if (lost)
+		tmc_etr_buf_insert_barrier_packet(etr_buf, offset);
+
+	tmc_etr_sync_perf_buffer(etr_perf, offset, size);
 
 	/*
 	 * In snapshot mode we simply increment the head by the number of byte
@@ -1518,7 +1542,6 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
 	if (etr_perf->snapshot)
 		handle->head += size;
 
-	lost |= etr_buf->full;
 out:
 	/*
 	 * Don't set the TRUNCATED flag in snapshot mode because 1) the
-- 
2.17.1

Re: [PATCH 2/2] coresight: tmc-etr: Add barrier packet when moving offset forward

[Leo Yan]

Hi Mathieu,

On Thu, Aug 22, 2019 at 04:09:15PM -0600, Mathieu Poirier wrote:
> This patch adds barrier packets in the trace stream when the offset in the
> data buffer needs to be moved forward.  Otherwise the decoder isn't aware
> of the break in the stream and can't synchronise itself with the trace
> data.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  .../hwtracing/coresight/coresight-tmc-etr.c   | 43 ++++++++++++++-----
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 4f000a03152e..0e4cd6ec5f28 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -946,10 +946,6 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
>  	WARN_ON(!etr_buf->ops || !etr_buf->ops->sync);
>  
>  	etr_buf->ops->sync(etr_buf, rrp, rwp);
> -
> -	/* Insert barrier packets at the beginning, if there was an overflow */
> -	if (etr_buf->full)
> -		tmc_etr_buf_insert_barrier_packet(etr_buf, etr_buf->offset);
>  }
>  
>  static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> @@ -1415,10 +1411,11 @@ static void tmc_free_etr_buffer(void *config)
>   * buffer to the perf ring buffer.
>   */
>  static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
> +				     unsigned long src_offset,
>  				     unsigned long to_copy)
>  {
>  	long bytes;
> -	long pg_idx, pg_offset, src_offset;
> +	long pg_idx, pg_offset;
>  	unsigned long head = etr_perf->head;
>  	char **dst_pages, *src_buf;
>  	struct etr_buf *etr_buf = etr_perf->etr_buf;
> @@ -1427,7 +1424,6 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf,
>  	pg_idx = head >> PAGE_SHIFT;
>  	pg_offset = head & (PAGE_SIZE - 1);
>  	dst_pages = (char **)etr_perf->pages;
> -	src_offset = etr_buf->offset + etr_buf->len - to_copy;
>  
>  	while (to_copy > 0) {
>  		/*
> @@ -1475,7 +1471,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>  		      void *config)
>  {
>  	bool lost = false;
> -	unsigned long flags, size = 0;
> +	unsigned long flags, offset, size = 0;
>  	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>  	struct etr_perf_buffer *etr_perf = config;
>  	struct etr_buf *etr_buf = etr_perf->etr_buf;
> @@ -1503,11 +1499,39 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
>  	size = etr_buf->len;
> +	offset = etr_buf->offset;
> +	lost |= etr_buf->full;
> +
> +	/*
> +	 * The ETR buffer may be bigger than the space available in the
> +	 * perf ring buffer (handle->size).  If so advance the offset so that we
> +	 * get the latest trace data.  In snapshot mode none of that matters
> +	 * since we are expected to clobber stale data in favour of the latest
> +	 * traces.
> +	 */
>  	if (!etr_perf->snapshot && size > handle->size) {
> -		size = handle->size;
> +		u32 mask = tmc_get_memwidth_mask(drvdata);
> +
> +		/*
> +		 * Make sure the new size is aligned in accordance with the
> +		 * requirement explained in function tmc_get_memwidth_mask().
> +		 */
> +		size = handle->size & mask;
> +		offset = etr_buf->offset + etr_buf->len - size;
> +
> +		if (offset >= etr_buf->size)
> +			offset -= etr_buf->size;
>  		lost = true;
>  	}
> -	tmc_etr_sync_perf_buffer(etr_perf, size);
> +
> +	/*
> +	 * Insert barrier packets at the beginning, if there was an overflow
> +	 * or if the offset had to be brought forward.
> +	 */
> +	if (lost)
> +		tmc_etr_buf_insert_barrier_packet(etr_buf, offset);
> +
> +	tmc_etr_sync_perf_buffer(etr_perf, offset, size);

With this new code, the inserting barrier packet has been moved out
from function tmc_sync_etr_buf(); but this patch doesn't handle the
path when user uses SysFS node to access trace data and the trace
buffer is also likely full, thus the SysFS mode might miss to insert
barrier packets?

Thanks,
Leo Yan

>  	/*
>  	 * In snapshot mode we simply increment the head by the number of byte
> @@ -1518,7 +1542,6 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>  	if (etr_perf->snapshot)
>  		handle->head += size;
>  
> -	lost |= etr_buf->full;
>  out:
>  	/*
>  	 * Don't set the TRUNCATED flag in snapshot mode because 1) the
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] coresight: Add barrier packet when moving offset forward

[Yabin Cui]

Tested-by: Yabin Cui <yabinc@google.com>

Re: [PATCH 0/2] coresight: Add barrier packet when moving offset forward

[Yabin Cui]

Thanks for fixing this problem. I didn't realize it because I usually use a
buffer size >= the default ETR buffer size, which is harder to reproduce the
problem.
The patches LGTM, maybe you also want to fix the problem commented by Leo Yan.
I tested the patches by recording etm data with a buffer size smaller than the
default ETR buffer size. Then I saw barrier packets when decoding with OpenCSD.
And I could decode successfully without error message.

Re: [PATCH 0/2] coresight: Add barrier packet when moving offset forward

[Mathieu Poirier]

Hi Yabin,

On Fri, 23 Aug 2019 at 18:30, Yabin Cui <yabinc@google.com> wrote:
>
> Thanks for fixing this problem. I didn't realize it because I usually use a
> buffer size >= the default ETR buffer size, which is harder to reproduce the
> problem.
> The patches LGTM, maybe you also want to fix the problem commented by Leo Yan.

I will look into the issue reported by Leo later today.

> I tested the patches by recording etm data with a buffer size smaller than the
> default ETR buffer size. Then I saw barrier packets when decoding with OpenCSD.
> And I could decode successfully without error message.

Can I add your Tested-by ?

Re: [PATCH 0/2] coresight: Add barrier packet when moving offset forward

[Yabin Cui]

> Can I add your Tested-by ?

Yes. I just sent a tested-by reply, but not sure if it works. I am not very familar
with linux kernel review system.