RE: [EXTERNAL] Re: [PATCH v8 5/7] coresight: tmc: Add support for reading crash data

[Linu Cherian]

Hi James,

> -----Original Message-----
> From: James Clark <james.clark@xxxxxxx>
> Sent: Friday, May 31, 2024 3:33 PM
> To: Linu Cherian <lcherian@xxxxxxxxxxx>
> 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>; Tanmay Jagdale
> <tanmay@xxxxxxxxxxx>; suzuki.poulose@xxxxxxx; mike.leach@xxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v8 5/7] coresight: tmc: Add support for
> reading crash data
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> 
> 
> On 31/05/2024 05:27, Linu Cherian wrote:
> > * Introduce a new mode CS_MODE_READ_CRASHDATA for reading trace
> >   captured in previous crash/watchdog reset.
> >
> > * Add special device files for reading ETR/ETF crash data.
> >
> > * User can read the crash data as below
> >
> >   For example, for reading crash data from tmc_etf sink
> >
> >   #dd if=/dev/crash_tmc_etfXX of=~/cstrace.bin
> >
> > Signed-off-by: Anil Kumar Reddy <areddy3@xxxxxxxxxxx>
> > Signed-off-by: Tanmay Jagdale <tanmay@xxxxxxxxxxx>
> > Signed-off-by: Linu Cherian <lcherian@xxxxxxxxxxx>
> > ---
> > Changelog from v7:
> > * Moved crash dev registration into new function,
> >   register_crash_dev_interface
> > * Removed redundant variable trace_addr in
> >   tmc_etr_setup_crashdata_buf
> >
> >  .../coresight/coresight-etm4x-core.c          |   1 +
> >  .../hwtracing/coresight/coresight-tmc-core.c  | 148 ++++++++++++++++-
> >  .../hwtracing/coresight/coresight-tmc-etf.c   |  73 +++++++++
> >  .../hwtracing/coresight/coresight-tmc-etr.c   | 152 +++++++++++++++++-
> >  drivers/hwtracing/coresight/coresight-tmc.h   |  11 +-
> >  include/linux/coresight.h                     |  13 ++
> >  6 files changed, 391 insertions(+), 7 deletions(-)
> >
> 
> [...]
> 
> > +static void register_crash_dev_interface(struct tmc_drvdata * drvdata,
> > +					 const char *name)
> > +{
> > +	drvdata->crashdev.name =
> > +		devm_kasprintf(&drvdata->csdev->dev, GFP_KERNEL,
> "%s_%s", "crash", name);
> > +	drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
> > +	drvdata->crashdev.fops = &tmc_crashdata_fops;
> > +	if (misc_register(&drvdata->crashdev))
> > +		dev_dbg(&drvdata->csdev->dev,
> > +			"Failed to setup user interface for crashdata\n"); }
> > +
> >  static int tmc_probe(struct amba_device *adev, const struct amba_id
> > *id)  {
> >  	int ret = 0;
> > @@ -619,6 +752,10 @@ static int tmc_probe(struct amba_device *adev,
> const struct amba_id *id)
> >  		coresight_unregister(drvdata->csdev);
> >  	else
> >  		pm_runtime_put(&adev->dev);
> > +
> > +	if (is_tmc_reserved_region_valid(dev))
> > +		register_crash_dev_interface(drvdata, desc.name);
> > +
> 
> Now that you've added an extra step to the probe you need to add a "goto
> out" when the previous step fails:
> 
>   if (ret) {
> 	coresight_unregister(drvdata->csdev);
> +	goto out;
>   }
> 
> That seems to be the pattern in the rest of the function. Otherwise you
> might register the interface on no device.
> 
> There's also a conflict here and in some other places. Since this has to go
> through the coresight branch you should base it off of coresight-next.
> 


Ack.

> >  out:
> >  	return ret;
> >  }
> > @@ -630,7 +767,8 @@ static void tmc_shutdown(struct amba_device
> *adev)
> >
> >  	spin_lock_irqsave(&drvdata->spinlock, flags);
> >
> > -	if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)
> > +	if ((coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)
> ||
> > +	    (coresight_get_mode(drvdata->csdev) ==
> CS_MODE_READ_CRASHDATA))
> >  		goto out;
> >
> >  	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) diff --git
> > a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index f9569585e9f8..655c0c0ba54b 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -657,6 +657,56 @@ static int tmc_panic_sync_etf(struct
> coresight_device *csdev)
> >  	return 0;
> >  }
> >
> 
> [...]
> 
> > @@ -725,6 +789,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata
> *drvdata)
> >  		__tmc_etb_disable_hw(drvdata);
> >  	}
> >
> > +mode_valid:
> >  	drvdata->reading = true;
> >  out:
> >  	spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -744,8
> +809,16
> > @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> >  			 drvdata->config_type != TMC_CONFIG_TYPE_ETF))
> >  		return -EINVAL;
> >
> > +
> 
> Whitespace change. There are a couple of other minor checkpatch style
> warnings.


Ack.

> 
> The rest looks good to me. All the tests are passing. I still think we should all
> the kself test for this mode, but that can be done later.


Sure. Will add kself tests for sysfs mode and send it as separate patches.