Re: [PATCH 1/4] coresight: tmc-etr: Advance buffer pointer in sync buffer.

Suzuki K Poulose
2021-04-26T10:42:30Z

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;
> bytes = min(bytes, (long)(PAGE_SIZE - pg_offset));
>
> 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 =
enable_hw()

emt_1 on -> ... -> enable_etr:
etr_perf->head =
already_enabled, skip enable_hw()

etm_2 on -> ... -> enable_etr:
etr_perf->head =
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