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

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

On 1/2/2026 6:55 PM, Bartosz Golaszewski wrote:
> On Fri, 26 Dec 2025 19:56:34 +0100, Shivendra Pratap
> <shivendra.pratap@oss.qualcomm.com> said:
>> 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.

[SNIP..]

>>
>> +struct sysfs_data {
> 
> Let's make this more descriptive? struct reboot_mode_sysfs_data?

Ack. thanks.

> 
>> +	const char *mode;
>> +	struct list_head list;
>> +};
>> +

[SNIP..]

>> +
>> +	reboot->reboot_mode_device = device_create(&reboot_mode_class, NULL, 0,
>> +						   (void *)head, reboot->dev->driver->name);
> 
> No, why pass the list? You should create an instance of struct sysfs_data per
> device_create(). If it needs to contain a list, then let it contain a list but
> don't allocate the list_head, that's really unusual.
> 

ok. Will create struct reboot_mode_sysfs_data with a list head and
allocate it as data.

>> +

[SNIP..]

>>
>> +static inline void reboot_mode_unregister_device(struct reboot_mode_driver *reboot)
>> +{
>> +	struct sysfs_data *sysfs_info;
>> +	struct sysfs_data *next;
>> +	struct list_head *head;
>> +
>> +	head = dev_get_drvdata(reboot->reboot_mode_device);
>> +	device_unregister(reboot->reboot_mode_device);
>> +	reboot->reboot_mode_device = NULL;
>> +
>> +	if (head) {
>> +		list_for_each_entry_safe(sysfs_info, next, head, list) {
>> +			list_del(&sysfs_info->list);
>> +			kfree_const(sysfs_info->mode);
>> +			kfree(sysfs_info);
>> +		}
> 
> This loop is duplicated, can you please factor it out into a dedicated
> function?

The loop frees the sysfs data. You mean i should directly call
reboot_mode_unregister_device in error path of reboot_mode_create_device
as not to duplicate the loop?

> 

[SNIP..]

>> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
>> index 4a2abb38d1d612ec0fdf05eb18c98b210f631b7f..b56783c32068096325f92445b9530d1856c4826c 100644
>> --- a/include/linux/reboot-mode.h
>> +++ b/include/linux/reboot-mode.h
>> @@ -5,6 +5,7 @@
>>  struct reboot_mode_driver {
>>  	struct device *dev;
>>  	struct list_head head;
>> +	struct device *reboot_mode_device;
> 
> Why can't this be part of struct (reboot_mode_)sysfs_data?
> 

If reboot_mode_device is kept in sysfs_data, we need a reference to free
it. Should I maintain reference for it in "reboot struct" and store 
sysfs_data pointer, so that it can be used to call device_unregister()?

Eg:
struct reboot 
{ 
..
..
  void *priv;
};

struct reboot_mode_sysfs_data {
    struct device *reboot_mode_device;
    struct list_head head;
};

---
data =  kzalloc(reboot_mode_sysfs_data);
reboot->priv = data;
--

thanks,
Shivendra