On 21/04/2021 13:04, Daniel Kiss wrote:
With polling the sync could called multiple times in a row. Without this change the data will be overwritten at the beginning of the buffer.
Signed-off-by: Daniel Kiss daniel.kiss@arm.com Signed-off-by: Branislav Rankov Branislav.Rankov@arm.com
drivers/hwtracing/coresight/coresight-tmc-etr.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index ea5027e397d02..dd19d1d1c3b38 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1442,7 +1442,7 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf, { long bytes; long pg_idx, pg_offset;
- unsigned long head = etr_perf->head;
- unsigned long head; char **dst_pages, *src_buf; struct etr_buf *etr_buf = etr_perf->etr_buf;
@@ -1465,7 +1465,7 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf, bytes = tmc_etr_buf_get_data(etr_buf, src_offset, to_copy, &src_buf); if (WARN_ON_ONCE(bytes <= 0))
break;
return;memcpy(dst_pages[pg_idx] + pg_offset, src_buf, bytes); @@ -1483,6 +1483,7 @@ static void tmc_etr_sync_perf_buffer(struct etr_perf_buffer *etr_perf, /* Move source pointers */ src_offset += bytes; }
- etr_perf->head = (pg_idx << PAGE_SHIFT) + pg_offset;
Looking at this patch, I feel the driver is doing a couple wrong things already.
1) We initialise etr_perf->head every time the ETR enable is called, irrespective of whether we actually try to enable the Hardware. e.g,
etm_0 on -> .. -> enable_etr : etr_perf->head = <head of the handle_0> enable_hw()
emt_1 on -> ... -> enable_etr: etr_perf->head = <head of the handle_1> already_enabled, skip enable_hw()
etm_2 on -> ... -> enable_etr: etr_perf->head = <head of the handle_2> already_enable, skip enable_hw()...
This doesn't look correct as we don't know which handle is going to get the data. This looks pointless.
2) Even more problematic is where we copy the AUX buffer content to. As mentioned above, we don't know which handle is going to be the last one to consume and we have a "etr_perf->head" that came from one of the handles and the "pages" that came from the first handle which created a etr_perf buffer. In sync_perf_buffer() we copy the hardware buffers to the "pages" (say of handle_0) with "etr_perf->head" (which could be from any other handle, say handle_2) and then we could return the number of bytes copied, which then is used to update the last handle (could be say handle_3), where there is no actual data copied.
To fix all of these issues, we must 1) Stop using etr_perf->head, and instead use the handle->head where we are called update_buffer on.
2) Keep track of the "pages" that belong to a given "handle" and then use those pages to copy the data to the current handle we are called to update the buffer on.
Did I get this wrong ?
Suzuki