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

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

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 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.
> 

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.

> 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.