Hi James,
-----Original Message----- From: James Clark james.clark@arm.com Sent: Friday, May 31, 2024 3:33 PM To: Linu Cherian lcherian@marvell.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; suzuki.poulose@arm.com; mike.leach@linaro.org 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@marvell.com Signed-off-by: Tanmay Jagdale tanmay@marvell.com Signed-off-by: Linu Cherian lcherian@marvell.com
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.