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

[James Clark <james.clark@arm.com>]

On 07/03/2024 03:36, 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@marvell.com>
> Signed-off-by: Tanmay Jagdale <tanmay@marvell.com>
> Signed-off-by: Linu Cherian <lcherian@marvell.com>
> ---
> Changelog from v6:
> * Removed read_prevboot flag in sysfs
> * Added special device files for reading crashdata 
> * Renamed CS mode READ_PREVBOOT to READ_CRASHDATA 
> * Setting the READ_CRASHDATA mode is done as part of file open.
> 

[...]

> @@ -619,6 +740,19 @@ 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))
> +		goto out;
> +
> +	drvdata->crashdev.name =
> +		devm_kasprintf(dev, GFP_KERNEL, "%s_%s", "crash", desc.name);
> +	drvdata->crashdev.minor = MISC_DYNAMIC_MINOR;
> +	drvdata->crashdev.fops = &tmc_crashdata_fops;
> +	ret = misc_register(&drvdata->crashdev);
> +	if (ret)
> +		pr_err("%s: Failed to setup dev interface for crashdata\n",
> +		       desc.name);
> +

Is this all optional after the is_tmc_reserved_region_valid()? Skipping
to out seems to be more like an error condition, but in this case it's
not? Having it like this makes it more difficult to add extra steps to
the probe function. You could move it to a function and flip the
condition which would be clearer:

   if (is_tmc_reserved_region_valid(dev))
      register_crash_dev_interface(drvdata);


>  out:
>  	return ret;
>  }

[...]

>  
> +static int tmc_etr_setup_crashdata_buf(struct tmc_drvdata *drvdata)
> +{
> +	int rc = 0;
> +	u64 trace_addr;
> +	struct etr_buf *etr_buf;
> +	struct etr_flat_buf *resrv_buf;
> +	struct tmc_crash_metadata *mdata;
> +	struct device *dev = &drvdata->csdev->dev;
> +
> +	mdata = drvdata->crash_mdata.vaddr;
> +
> +	etr_buf = kzalloc(sizeof(*etr_buf), GFP_ATOMIC);
> +	if (!etr_buf) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +	etr_buf->size = drvdata->crash_tbuf.size;
> +
> +	resrv_buf = kzalloc(sizeof(*resrv_buf), GFP_ATOMIC);
> +	if (!resrv_buf) {
> +		rc = -ENOMEM;
> +		goto rmem_err;
> +	}
> +
> +	/*
> +	 * Buffer address given by metadata for retrieval of trace data
> +	 * from previous boot is expected to be same as the reserved
> +	 * trace buffer memory region provided through DTS
> +	 */
> +	if (is_tmc_reserved_region_valid(dev->parent)
> +	    && (drvdata->crash_tbuf.paddr == mdata->trc_paddr))
> +		resrv_buf->vaddr = drvdata->crash_tbuf.vaddr;
> +	else {
> +		dev_dbg(dev, "Trace buffer address of previous boot invalid\n");
> +		rc = -EINVAL;
> +		goto map_err;
> +	}
> +
> +	resrv_buf->size = etr_buf->size;
> +	resrv_buf->dev = &drvdata->csdev->dev;
> +	etr_buf->hwaddr = trace_addr;

trace_addr is uninitialised at this point. If you pull the latest
coresight branch we enabled some extra compiler warnings which catch this.

I assume it's supposed to be mdata->trc_paddr?

Is there some kind of test that could be added that could have caught
this? Maybe skip the full reboot flow but just enable the feature and
see if we can read from the buffer.

Or even just toggling the mode on and off via sysfs. We're running the
Perf and kselftests on Juno internally so I can add a reserved region to
the Juno DT and make sure this stays working.