Hi James,
-----Original Message----- From: James Clark james.clark@arm.com Sent: Monday, April 22, 2024 1:48 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 21/04/2024 03:49, Linu Cherian wrote:
Hi James,
-----Original Message----- From: James Clark james.clark@arm.com Sent: Monday, April 15, 2024 2:59 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: Re: [EXTERNAL] Re: [PATCH v7 5/7] coresight: tmc: Add support for reading crash data
On 15/04/2024 05:01, Linu Cherian wrote:
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 forcrashdata\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);
Did you meant changing the condition of "is_tmc_reserved_region_valid"
by "flip the condition".
If yes, that’s not required IMHO, since the reserved region is still valid.
By flip I mean remove the !. You had this:
if (!is_tmc_reserved_region_valid(dev)) goto out;
But instead you should put your registration code in a function, remove the ! and replace the goto with a function:
if (is_tmc_reserved_region_valid(dev)) ret = register_crash_dev_interface(drvdata);Where register_crash_dev_interface() is everything you added in between the goto and the out: label. The reason is that you've made it impossible to extend the probe function with new behavior without having to understand that this new bit must always come last. Otherwise new behavior would also be skipped over if the reserved region doesn't exist.
Thanks. That’s clear to me.
IIUC, the idea here is to not to fail the tmc_probe due to an error condition in register_crash_dev_interface, so that the normal condition is
not affected. Also the error condition can be notified to the user using a pr_dbg / pr_err.
Thanks.
I'm not sure I follow exactly what you mean here, but for the one error condition you are checking for on the call to misc_register() you can still return that from the new function and check it in the probe.
Actually was trying to clarify that we may not want to fail the probe due to a failure in the register_crash_dev_interface, since the normal trace operations could continue without crash_dev interface.(Tracing with or without the reserved region doesn’t get affected as well). Please see the changes below. That way the changes are simpler.
@@ -507,6 +628,18 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev) return burst_size; }
+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); + out: return ret; }
Thanks. Linu Cherian.