RE: [EXTERNAL] Re: [PATCH v7 2/7] coresight: tmc-etr: Add support to use reserved trace memory

[Linu Cherian]

Hi James,

> -----Original Message-----
> From: James Clark <james.clark@xxxxxxx>
> Sent: Friday, April 12, 2024 3:27 PM
> To: Linu Cherian <lcherian@xxxxxxxxxxx>; suzuki.poulose@xxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; coresight@xxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Sunil Kovvuri Goutham
> <sgoutham@xxxxxxxxxxx>; George Cherian <gcherian@xxxxxxxxxxx>; Anil
> Kumar Reddy H <areddy3@xxxxxxxxxxx>; mike.leach@xxxxxxxxxx;
> leo.yan@xxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v7 2/7] coresight: tmc-etr: Add support to
> use reserved trace memory
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> 
> 
> On 07/03/2024 03:36, Linu Cherian wrote:
> > Add support to use reserved memory for coresight ETR trace buffer.
> >
> > Introduce a new ETR buffer mode called ETR_MODE_RESRV, which becomes
> > available when ETR device tree node is supplied with a valid reserved
> > memory region.
> >
> > ETR_MODE_RESRV can be selected only by explicit user request.
> >
> > $ echo resrv
> >/sys/bus/coresight/devices/tmc_etr<N>/buf_mode_preferred
> >
> > Signed-off-by: Anil Kumar Reddy <areddy3@xxxxxxxxxxx>
> > Signed-off-by: Linu Cherian <lcherian@xxxxxxxxxxx>
> > ---
> > Changelog from v6:
> > * Removed redundant goto statements
> > * Setting of etr_buf->size to the reserved memory size is done
> >   after successful dma map inside the alloc function
> > * Removed the special casing for ETR_MODE_RESRV
> > * Fixed the tab spacing in struct tmc_drvdata
> >
> >  .../hwtracing/coresight/coresight-tmc-core.c  | 47 +++++++++++
> >  .../hwtracing/coresight/coresight-tmc-etr.c   | 82 ++++++++++++++++++-
> >  drivers/hwtracing/coresight/coresight-tmc.h   | 27 ++++++
> >  3 files changed, 153 insertions(+), 3 deletions(-)
> >
> 
> [...]
> 
> >  static bool etr_can_use_flat_mode(struct etr_buf_hw *buf_hw, ssize_t
> > etr_buf_size) @@ -874,13 +947,10 @@ static struct etr_buf
> *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata,
> >  	if (!etr_buf)
> >  		return ERR_PTR(-ENOMEM);
> >
> > -	etr_buf->size = size;
> > -
> 
> Hi Linu,
> 
> Not sure if this was left in by mistake? It's not mentioned in the commit
> message and it doesn't seem to match the description.
> 

Yeah, that change was by mistake. Sorry about that.

> Please make sure the current tests pass both with and without a reserved
> buffer defined in the DT. I get lots of failures with this patchset applied on
> N1SDP. ETF seems to work but ETR doesn't:
> 

Ack.

>   $ sudo perf test -vvv "arm coresight"
> 
>   Recording trace (only user mode) with path: CPU1 => tmc_etf0
>   CoreSight path testing (CPU1 -> tmc_etf0): PASS
> 
>   Recording trace (only user mode) with path: CPU1 => tmc_etr0
>   CoreSight path testing (CPU1 -> tmc_etr0): FAIL
>   ...
> 
> Dmesg:
>   [ 1938.622091] coresight tmc_etr0: Unable to allocate ETR buffer
> 
> >  	/* If there is user directive for buffer mode, try that first */
> >  	if (drvdata->etr_mode != ETR_MODE_AUTO)
> >  		rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata,
> >  					    etr_buf, node, pages);
> > -
> 
> Whitespace change.