Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 27 Aug 2019 09:30:38 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     suzuki.poulose@arm.com, mike.leach@arm.com, yabinc@google.com,
        alexander.shishkin@linux.intel.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] coresight: tmc-etr: Decouple buffer sync and
 barrier packet insertion
Message-ID: <20190827013038.GA4596@leoy-ThinkPad-X240s>
References: <20190826194605.3791-1-mathieu.poirier@linaro.org>
 <20190826194605.3791-3-mathieu.poirier@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190826194605.3791-3-mathieu.poirier@linaro.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Aug 26, 2019 at 01:46:04PM -0600, Mathieu Poirier wrote:
> If less space is available in the perf ring buffer than the ETR buffer,
> barrier packets inserted in the trace stream by tmc_sync_etr_buf() are
> skipped over when the head of the buffer is moved forward, resulting in
> traces that can't be decoded.
> 
> This patch decouples the process of syncing ETR buffers and the addition
> of barrier packets in order to perform the latter once the offset in the
> trace buffer has been properly computed.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  .../hwtracing/coresight/coresight-tmc-etr.c    | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 4f000a03152e..bae47272de98 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)
> @@ -1086,6 +1082,13 @@ static void tmc_etr_sync_sysfs_buf(struct tmc_drvdata *drvdata)
>  		drvdata->sysfs_buf = NULL;
>  	} else {
>  		tmc_sync_etr_buf(drvdata);
> +		/*
> +		 * 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);
>  	}
>  }
>  
> @@ -1502,11 +1505,16 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
>  	CS_LOCK(drvdata->base);
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
> +	lost = etr_buf->full;

Comparing to the previous version, it drops '|' bitwise operator;
seems to me this is more neat :)

I think Yabin's testing is more convinced, so I skip to test.
FWIW, these three patches look good to me:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

>  	size = etr_buf->len;
>  	if (!etr_perf->snapshot && size > handle->size) {
>  		size = handle->size;
>  		lost = true;
>  	}
> +
> +	/* Insert barrier packets at the beginning, if there was an overflow */
> +	if (lost)
> +		tmc_etr_buf_insert_barrier_packet(etr_buf, etr_buf->offset);
>  	tmc_etr_sync_perf_buffer(etr_perf, size);
>  
>  	/*
> @@ -1517,8 +1525,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
>