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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: 
 <PH0PR18MB5002CFB5DD77312CE0337896CE132@PH0PR18MB5002.namprd18.prod.outlook.com>
Date: Sun, 21 Apr 2024 02:49:18 +0000
From: Linu Cherian <lcherian@...vell.com>
To: James Clark <james.clark@....com>,
        Suzuki K Poulose
	<suzuki.poulose@....com>
CC: "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
        "coresight@...ts.linaro.org"
	<coresight@...ts.linaro.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        George Cherian <gcherian@...vell.com>,
        Anil Kumar Reddy H
	<areddy3@...vell.com>,
        Tanmay Jagdale <tanmay@...vell.com>,
        "mike.leach@...aro.org" <mike.leach@...aro.org>,
        "leo.yan@...aro.org"
	<leo.yan@...aro.org>
Subject: Re: [PATCH v7 5/7] coresight: tmc: Add support for reading crash data

Hi James,

> -----Original Message-----
> From: James Clark <james.clark@....com>
> Sent: Monday, April 15, 2024 2:59 PM
> To: Linu Cherian <lcherian@...vell.com>; Suzuki K Poulose
> <suzuki.poulose@....com>
> Cc: linux-arm-kernel@...ts.infradead.org; coresight@...ts.linaro.org; linux-
> kernel@...r.kernel.org; robh+dt@...nel.org;
> krzysztof.kozlowski+dt@...aro.org; conor+dt@...nel.org;
> devicetree@...r.kernel.org; Sunil Kovvuri Goutham
> <sgoutham@...vell.com>; George Cherian <gcherian@...vell.com>; Anil
> Kumar Reddy H <areddy3@...vell.com>; Tanmay Jagdale
> <tanmay@...vell.com>; mike.leach@...aro.org; leo.yan@...aro.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@....com>
> >> Sent: Friday, April 12, 2024 3:36 PM
> >> To: Linu Cherian <lcherian@...vell.com>; Suzuki K Poulose
> >> <suzuki.poulose@....com>
> >> Cc: linux-arm-kernel@...ts.infradead.org; coresight@...ts.linaro.org;
> >> linux- kernel@...r.kernel.org; robh+dt@...nel.org;
> >> krzysztof.kozlowski+dt@...aro.org; conor+dt@...nel.org;
> >> devicetree@...r.kernel.org; Sunil Kovvuri Goutham
> >> <sgoutham@...vell.com>; George Cherian <gcherian@...vell.com>;
> Anil
> >> Kumar Reddy H <areddy3@...vell.com>; Tanmay Jagdale
> >> <tanmay@...vell.com>; mike.leach@...aro.org; leo.yan@...aro.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@...vell.com>
> >>> Signed-off-by: Tanmay Jagdale <tanmay@...vell.com>
> >>> Signed-off-by: Linu Cherian <lcherian@...vell.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);
> >>

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.

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.