Re: [PATCH v14 3/6] reboot: add parsable tokens for power state change reasons

[Faruque Ansari <faruque.ansari@oss.qualcomm.com>]

Hi Matti Vaittinen,

On 22-Sep-26 1:07 PM, Matti Vaittinen wrote:
> On 21/09/2026 20:44, Faruque Ansari wrote:
>> psc_reason_to_str() returns human-readable labels that contain 
>> spaces
>> (e.g. "over temperature"). Those are fine for logs but unusable as
>> values in a space-separated sysfs list or as a write target.
>>
>> Extend the single reason descriptor table with a stable, space- 
>> free
>> token next to the existing label, and add psc_reason_to_token() 
>> and
>> psc_reason_from_token() so consumers can emit and parse reasons 
>> without
>> inventing their own string table.
>>
>> Co-developed-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> Signed-off-by: Faruque Ansari <faruque.ansari@oss.qualcomm.com>
>>
>> changes v12:
>> - new patch
>> ---
>>   include/linux/reboot.h |  2 ++
>>   kernel/reboot.c        | 77 +++++++++++++++++++++++++++++++++ 
>> ++++++-----------
>>   2 files changed, 63 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
>> index 08a7549bbc40..4c5327dd7645 100644
>> --- a/include/linux/reboot.h
>> +++ b/include/linux/reboot.h
>> @@ -244,6 +244,8 @@ enum psc_reason {
>>   #define PSCR_MAX_REASON    (PSCR_REASON_COUNT - 1)
>>   const char *psc_reason_to_str(enum psc_reason reason);
>> +const char *psc_reason_to_token(enum psc_reason reason);
>> +int psc_reason_from_token(const char *token, enum psc_reason 
>> *reason);
>>   /**
>>    * enum hw_protection_action - Hardware protection action
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index d5a45db82c3d..e53fb31bad06 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -1084,34 +1084,79 @@ void set_psc_reason(enum psc_reason 
>> reason)
>>   }
>>   EXPORT_SYMBOL_GPL(set_psc_reason);
>> -static const char * const pscr_reason_strs[] = {
>> -    [PSCR_UNKNOWN]            = POWER_ON_REASON_UNKNOWN,
>> -    [PSCR_UNDER_VOLTAGE]      = POWER_ON_REASON_BROWN_OUT,
>> -    [PSCR_OVER_CURRENT]       = POWER_ON_REASON_OVER_CURRENT,
>> -    [PSCR_REGULATOR_FAILURE]  = 
>> POWER_ON_REASON_REGULATOR_FAILURE,
>> -    [PSCR_OVER_TEMPERATURE]   = POWER_ON_REASON_OVER_TEMPERATURE,
>> -    [PSCR_EC_PANIC]           = POWER_ON_REASON_EC_PANIC,
>> +/**
>> + * struct psc_reason_desc - Descriptor for a power state 
>> change reason.
>> + * @token: Stable, parsable identifier without spaces (e.g. 
>> "over-temperature").
>> + *       Suitable for use in sysfs values and as a user/kernel 
>> contract.
>> + * @label: Human-readable description (e.g. "over 
>> temperature"), for logs.
>> + */
>> +struct psc_reason_desc {
>> +    const char *token;
>> +    const char *label;
>> +};
>> +
>> +static const struct psc_reason_desc psc_reason_descs[] = {
>> +    [PSCR_UNKNOWN]         = { "unknown",          
>> POWER_ON_REASON_UNKNOWN },
>> +    [PSCR_UNDER_VOLTAGE]     = { "under-voltage",      
>> POWER_ON_REASON_BROWN_OUT },
>> +    [PSCR_OVER_CURRENT]     = { "over-current",      
>> POWER_ON_REASON_OVER_CURRENT },
>> +    [PSCR_REGULATOR_FAILURE] = { "regulator-failure", 
>> POWER_ON_REASON_REGULATOR_FAILURE },
>> +    [PSCR_OVER_TEMPERATURE]     = { "over-temperature",  
>> POWER_ON_REASON_OVER_TEMPERATURE },
>> +    [PSCR_EC_PANIC]         = { "ec-panic",      
>> POWER_ON_REASON_EC_PANIC },
>>   };
> 
> nit:
> Is there a reason for having the label as a define while token is 
> a plain string? It may be just me, but I would have expected the 
> userland contract to be defined in a header. (It may be just my 
> false expectation though!).

Thanks for the review.

On token vs. label: I'd prefer to keep token as a plain literal 
in reboot.c. Unlike label, which mirrors the shared 
POWER_ON_REASON_* definitions and is used by code outside 
reboot.c, token is introduced by this patch and is only produced 
and consumed by psc_reason_to_token() and psc_reason_from_token() 
within this file.
Since there are no external users that need the string to be 
shared, moving it to a header doesn't provide additional benefit.

> 
>>   /**
>> - * psc_reason_to_str - Converts a power state change reason 
>> enum to a string.
>> - * @reason: The `psc_reason` enum value to be converted.
>> - *
>> - * This function provides a human-readable string 
>> representation of the power
>> - * state change reason, making it easier to interpret logs and 
>> debug messages.
>> + * psc_reason_to_str - Human-readable label for a power state 
>> change reason.
>> + * @reason: The `psc_reason` value to convert.
>>    *
>> - * Return:
>> - * - A string corresponding to the given `psc_reason` value.
>> - * - `"Invalid"` if the value is not recognized.
>> + * Return: The label string, or "Invalid" if @reason is out of 
>> range. For a
>> + * stable, parsable form use psc_reason_to_token() instead.
>>    */
>>   const char *psc_reason_to_str(enum psc_reason reason)
>>   {
>>       if (reason < 0 || reason >= PSCR_REASON_COUNT)
>>           return "Invalid";
>> -    return pscr_reason_strs[reason];
>> +    return psc_reason_descs[reason].label;
> 
> nit:
> I would feel a tad safer if the psc_reason_descs[] was always 
> guaranteed to be initialized up-to PSCR_REASON_COUNT. Having the 
> enum and PSCR_REASON_COUNT defined in one place, and 
> psc_reason_descs[] initialized here, adds a window for: "I added 
> new enum value, forgot to update the psc_reason_descs[]". Can we 
> add a NULL check or are we on a very performance critical path here?
> 

Thanks for pointing this out. I'll fix it in the next revision.

>>   }
>>   EXPORT_SYMBOL_GPL(psc_reason_to_str);
>> +/**
>> + * psc_reason_to_token - Stable, parsable token for a power 
>> state change reason.
>> + * @reason: The `psc_reason` value to convert.
>> + *
>> + * Return: The token string (no spaces), or "invalid" if 
>> @reason is out of
>> + * range. Round-trips with psc_reason_from_token().
>> + */
>> +const char *psc_reason_to_token(enum psc_reason reason)
>> +{
>> +    if (reason < 0 || reason >= PSCR_REASON_COUNT)
>> +        return "invalid";
>> +    return psc_reason_descs[reason].token;
> 
> Please, see my comment above.
> 
>> +}
>> +EXPORT_SYMBOL_GPL(psc_reason_to_token);
>> +
>> +/**
>> + * psc_reason_from_token - Parse a reason token into a 
>> `psc_reason` value.
>> + * @token: A token as returned by psc_reason_to_token(). A 
>> trailing newline is
>> + *       tolerated.
>> + * @reason: Output; set on success.
>> + *
>> + * Return: 0 on success or -EINVAL if @token matches no known 
>> reason.
>> + */
>> +int psc_reason_from_token(const char *token, enum psc_reason 
>> *reason)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < PSCR_REASON_COUNT; i++) {
>> +        if (sysfs_streq(token, psc_reason_descs[i].token)) {
> 
> I suppose my comment applies here as well.
> 
>> +            *reason = i;
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(psc_reason_from_token);
>> +
>>   /**
>>    * __hw_protection_trigger - Trigger an emergency system 
>> shutdown or reboot
>>    *
> 
> After all these years in the business - it's hard to be as 
> confident as I used to. ;) So, I won't say [aloud] that my 
> preferred way is the only correct way. Please weigh my comments 
> and decide as you see best, this is "good enough" for me if you 
> don't agree with my suggestions.

I agree with your suggestion and will incorporate it in the next 
version.

Thanks,
Faruque Ansari

> 
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Yours,
>    -- Matti
>