Re: [PATCH v21 2/2] power: reset: reboot-mode: Expose sysfs for registered reboot_modes

[Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>]

On 12/22/2025 10:56 PM, Bartosz Golaszewski wrote:
> On Mon, Dec 22, 2025 at 4:33 AM Shivendra Pratap
> <shivendra.pratap@oss.qualcomm.com> wrote:
>>
>> Currently, there is no standardized mechanism for userspace to discover
>> which reboot-modes are supported on a given platform. This limitation
>> forces tools and scripts to rely on hardcoded assumptions about the
>> supported reboot-modes.
>>
>> Create a class 'reboot-mode' and a device under it to expose a sysfs
>> interface to show the available reboot mode arguments to userspace. Use
>> the driver_name field of the struct reboot_mode_driver to create the
>> device.  For device-based drivers, configure the device driver name as
>> driver_name.
>>
>> This results in the creation of:
>>   /sys/class/reboot-mode/<driver>/reboot_modes
>>
>> This read-only sysfs file will exposes the list of supported reboot
>> modes arguments provided by the driver, enabling userspace to query the
>> list of arguments.
>>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
> 
> [snip]
> 
>> +
>>  static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>>                                           const char *cmd)
>>  {
>> @@ -76,6 +109,15 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>         size_t len = strlen(PREFIX);
>>         int ret;
>>
>> +       reboot->reboot_mode_device = device_create(&reboot_mode_class, NULL, 0,
>> +                                                  (void *)reboot, reboot->driver_name);
> 
> You should define a separate struct in this file and pass it as
> drvdata as argument 4. The main reason for using device_create() was
> to not have to store any data associated with the sysfs ABI in a
> separate struct, not in the public one.

Currently we directly access list head in show_modes.
We want to maintain a separate struct and copy the mode strings
there after registration is complete and then set drvdata and the same
in show_modes? 

> 
>> +       if (IS_ERR(reboot->reboot_mode_device)) {
>> +               ret = PTR_ERR(reboot->reboot_mode_device);
>> +               reboot->reboot_mode_device = NULL;
>> +               return ret;
>> +       }
>> +
>> +       mutex_init(&reboot->reboot_mode_mutex);
> 
> Add a corresponding mutex_destroy() please.

Ack. thanks.

> 
> [snip]
> 
>> +
>>  MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
>>  MODULE_DESCRIPTION("System reboot mode core library");
>>  MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
>> index 4a2abb38d1d612ec0fdf05eb18c98b210f631b7f..d7141a1a609b62bd3185642ecc1478fdd3555037 100644
>> --- a/include/linux/reboot-mode.h
>> +++ b/include/linux/reboot-mode.h
>> @@ -2,9 +2,15 @@
>>  #ifndef __REBOOT_MODE_H__
>>  #define __REBOOT_MODE_H__
>>
>> +#include <linux/mutex.h>
>> +
>>  struct reboot_mode_driver {
>>         struct device *dev;
>>         struct list_head head;
>> +       const char *driver_name;
> 
> I have no idea why you're storing the name here.
> 
> As I said above: you should not need to modify this structure (if
> maybe for the mutex if modifications of this struct from existing code
> can race with the sysfs code). Use a separate one for sysfs data.
> 

ok. If separate data for mode strings is maintained, may be we won't
need a mutex lock as-well.

>> +       struct device *reboot_mode_device;
> 
> I think you misunderstood my comment about the renaming: what I meant
> was: propose to rename the existing reboot_mode_driver structure to
> reboot_mode_device because this is what it is in reality.

reboot_mode_device - this is not renamed. variable name is same as in
previous version.

thanks,
Shivendra