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

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Daniel Kiss <daniel.kiss@arm.com>,
	mathieu.poirier@linaro.org, mike.leach@linaro.org,
	leo.yan@linaro.org, coresight@lists.linaro.org,
	linux-arm-kernel@lists.infradead.org
Cc: denik@google.com, Branislav Rankov <Branislav.Rankov@arm.com>
Subject: Re: [PATCH 1/4] coresight: tmc-etr: Advance buffer pointer in sync buffer.
Date: Mon, 26 Apr 2021 11:40:44 +0100	[thread overview]
Message-ID: <bd9de3ab-14f7-2659-0dc8-619bca0f52d7@arm.com> (raw)
In-Reply-To: <20210421120413.3110775-2-daniel.kiss@arm.com>

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 = <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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel