Hi James,
-----Original Message----- From: James Clark james.clark@arm.com Sent: Friday, April 12, 2024 3:36 PM To: Linu Cherian lcherian@marvell.com; Suzuki K Poulose suzuki.poulose@arm.com Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org; Sunil Kovvuri Goutham sgoutham@marvell.com; George Cherian gcherian@marvell.com; Anil Kumar Reddy H areddy3@marvell.com; Tanmay Jagdale tanmay@marvell.com; mike.leach@linaro.org; leo.yan@linaro.org Subject: [EXTERNAL] Re: [PATCH v7 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 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:
Ack.
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 bootinvalid\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?
Since no DMA operation happens here, its supposed to be kept NULL. Since it was redundant for crash data read operation, missed catching this. Will fix this.
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.
As pointed above this field is redundant for crashdata read mode. So its not a functionality breakage as such.
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.
Did you meant adding a kselftest for tracing and reading back tracedata in sysfs mode using the reserved region ?
Thanks Linu Cherian.