Re: [PATCH v4 2/3] power: reset: pscrr: add watchdog pretimeout reason tracking

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

On 10/12/26 13:06, Faruque Ansari wrote:
> 
> Hi Daniel Lezcano,
> 
> On 07-Oct-26 4:24 PM, Daniel Lezcano wrote:
>> On 8/5/26 10:54, Faruque Ansari wrote:
>>> Watchdog pretimeout resets are not recorded with a dedicated reason,
>>> causing subsequent boots to report PSCR_UNKNOWN and making it difficult
>>> to distinguish them from other unexpected resets.
>>>
>>> Add PSCR_WATCHDOG_PRETIMEOUT as a dedicated reason code and prevent the
>>> panic notifier from overwriting a watchdog pretimeout reason with
>>> PSCR_KERNEL_PANIC when the pretimeout governor triggers a panic.
>>>
>>> Signed-off-by: Faruque Ansari <faruque.ansari@oss.qualcomm.com>
>>> ---
>>>   drivers/power/reset/pscrr/pscrr.c     | 11 +++++++----
>>>   include/linux/power/power_on_reason.h |  1 +
>>>   include/linux/reboot.h                |  4 ++++
>>>   kernel/reboot.c                       |  1 +
>>>   4 files changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/power/reset/pscrr/pscrr.c b/drivers/power/ 
>>> reset/pscrr/pscrr.c
>>> index 8c45b84059a9..b7e6239e207e 100644
>>> --- a/drivers/power/reset/pscrr/pscrr.c
>>> +++ b/drivers/power/reset/pscrr/pscrr.c
>>> @@ -529,12 +529,15 @@ static struct notifier_block pscrr_reboot_nb = {
>>>   static int pscrr_panic_notifier(struct notifier_block *nb,
>>>                   unsigned long action, void *unused)
>>>   {
>>> -    struct pscrr_provider_dir *dir;
>>> -    set_psc_reason(PSCR_KERNEL_PANIC);
>>> +    /*
>>> +     * Do not overwrite a watchdog pretimeout reason already set by the
>>> +     * pretimeout path before it triggered this panic.
>>> +     */
>>> +    if (get_psc_reason() != PSCR_WATCHDOG_PRETIMEOUT)
>>> +        set_psc_reason(PSCR_KERNEL_PANIC);
>>
>> It does not sounds correct.
 >>>> May be you should move the set_psc_reason() to different places for
>> panic.
>>
>> I did not explore the panic() function but I would suggest to have 
>> something like:
>>
>> panic(..., ) --> defaults to PSCR_KERNEL_PANIC
>>
>> panic_with_pscrr(..., reason)
>>
>> Then pscrr_panic_notifier() only does the record
> 
> Thanks for your review.
> 
> I agree that handling the kernel panic reason in this notifier may not 
> be the ideal approach today. However, my intention is to add userspace 
> crash recording support in the future as well. Since this notifier 
> already serves as the common point for recording restart reasons, it 
> seems like the most appropriate place to handle userspace-triggered 
> reboot reasons when that support is added.
> 
> For consistency, I added the handling for kernel panic and watchdog 
> reset reasons here as well. While they are not strictly required for the 
> current use case, this notifier is the most suitable place to record 
> reboot reasons. Keeping all reboot reason handling in one place will 
> also make it easier to add userspace reboot/crash recording support in 
> the future, without introducing a separate mechanism later.

I understand the purpose of the series, but setting the reboot reason 
directly from this notifier does not seem like the right approach.

The notifier should be responsible for recording the reason, while 
determining and updating that reason should be handled independently, as 
it may originate from different execution contexts.

The need to explicitly check for PSCR_WATCHDOG_PRETIMEOUT before setting 
PSCR_KERNEL_PANIC illustrates the problem: the notifier now needs to 
know about other reboot reasons and their relative priorities.

As more reboot reasons are introduced, this approach will require 
additional special cases and become increasingly difficult to maintain.

Wouldn't it make more sense to centralize the reason selection policy, 
keeping the notifier focused on recording the selected reason?