by Linu Cherian

[permalink] [raw]

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

Hi James,

> -----Original Message-----
> From: James Clark <[email protected]>
> Sent: Monday, April 22, 2024 1:48 PM
> To: Linu Cherian <[email protected]>; Suzuki K Poulose
> <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Sunil Kovvuri Goutham
> <[email protected]>; George Cherian <[email protected]>; Anil
> Kumar Reddy H <[email protected]>; Tanmay Jagdale
> <[email protected]>; [email protected]; [email protected]
> 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 <[email protected]>
> >> Sent: Monday, April 15, 2024 2:59 PM
> >> To: Linu Cherian <[email protected]>; Suzuki K Poulose
> >> <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> linux- [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; Sunil Kovvuri Goutham
> >> <[email protected]>; George Cherian <[email protected]>;
> Anil
> >> Kumar Reddy H <[email protected]>; Tanmay Jagdale
> >> <[email protected]>; [email protected]; [email protected]
> >> 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 <[email protected]>
> >>>> Sent: Friday, April 12, 2024 3:36 PM
> >>>> To: Linu Cherian <[email protected]>; Suzuki K Poulose
> >>>> <[email protected]>
> >>>> Cc: [email protected];
> >>>> [email protected];
> >>>> linux- [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; Sunil Kovvuri Goutham
> >>>> <[email protected]>; George Cherian <[email protected]>;
> >> Anil
> >>>> Kumar Reddy H <[email protected]>; Tanmay Jagdale
> >>>> <[email protected]>; [email protected]; [email protected]
> >>>> 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 <[email protected]>
> >>>>> Signed-off-by: Tanmay Jagdale <[email protected]>
> >>>>> Signed-off-by: Linu Cherian <[email protected]>
> >>>>> ---
> >>>>> 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.
>

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.

2024-04-25 09:33:04

by James Clark

[permalink] [raw]

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

On 25/04/2024 03:07, Linu Cherian wrote:
> Hi James,
>
>> -----Original Message-----
>> From: James Clark <[email protected]>
>> Sent: Monday, April 22, 2024 1:48 PM
>> To: Linu Cherian <[email protected]>; Suzuki K Poulose
>> <[email protected]>
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; Sunil Kovvuri Goutham
>> <[email protected]>; George Cherian <[email protected]>; Anil
>> Kumar Reddy H <[email protected]>; Tanmay Jagdale
>> <[email protected]>; [email protected]; [email protected]
>> 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 <[email protected]>
>>>> Sent: Monday, April 15, 2024 2:59 PM
>>>> To: Linu Cherian <[email protected]>; Suzuki K Poulose
>>>> <[email protected]>
>>>> Cc: [email protected]; [email protected];
>>>> linux- [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; Sunil Kovvuri Goutham
>>>> <[email protected]>; George Cherian <[email protected]>;
>> Anil
>>>> Kumar Reddy H <[email protected]>; Tanmay Jagdale
>>>> <[email protected]>; [email protected]; [email protected]
>>>> 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 <[email protected]>
>>>>>> Sent: Friday, April 12, 2024 3:36 PM
>>>>>> To: Linu Cherian <[email protected]>; Suzuki K Poulose
>>>>>> <[email protected]>
>>>>>> Cc: [email protected];
>>>>>> [email protected];
>>>>>> linux- [email protected]; [email protected];
>>>>>> [email protected]; [email protected];
>>>>>> [email protected]; Sunil Kovvuri Goutham
>>>>>> <[email protected]>; George Cherian <[email protected]>;
>>>> Anil
>>>>>> Kumar Reddy H <[email protected]>; Tanmay Jagdale
>>>>>> <[email protected]>; [email protected]; [email protected]
>>>>>> 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 <[email protected]>
>>>>>>> Signed-off-by: Tanmay Jagdale <[email protected]>
>>>>>>> Signed-off-by: Linu Cherian <[email protected]>
>>>>>>> ---
>>>>>>> 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.
>>
>
> 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.

Looks good to me!