Message-ID: <dda61a49-90b1-ef48-5a46-d6953cd5375a@oss.qualcomm.com>
Date: Wed, 26 Nov 2025 22:18:32 +0530
From: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-pm@...r.kernel.org, Sebastian Reichel <sre@...nel.org>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>
Subject: Re: [PATCH v19 2/2] power: reset: reboot-mode: Expose sysfs for
 registered reboot_modes



On 11/25/2025 8:14 PM, Bartosz Golaszewski wrote:
> On Sat, 22 Nov 2025 19:05:45 +0100, Shivendra Pratap
> <shivendra.pratap@....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.
>>
>> 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@....qualcomm.com>
>> ---
>>  drivers/power/reset/reboot-mode.c | 86 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/reboot-mode.h       |  3 ++
>>  2 files changed, 89 insertions(+)
>>
>> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
>> index fba53f638da04655e756b5f8b7d2d666d1379535..71cd97ffb3ee8da530514ab918bc6d58a122b4ce 100644
>> --- a/drivers/power/reset/reboot-mode.c
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -13,12 +13,61 @@
>>
>>  #define PREFIX "mode-"
>>
>> +static bool reboot_mode_class_registered;
> 
> You don't need this, please see below.

reboot_mode_class_registered was used for two reason.
one is resolved: will directly call class_unregister.

for second : If class_register fails, we want don't call register device
in reboot_mode_register.

at -
	if (reboot_mode_class_registered)
		reboot_mode_register_device(reboot);
 

> 
>> +
>>  struct mode_info {
>>  	const char *mode;
>>  	u32 magic;
>>  	struct list_head list;
>>  };
>>
>> +static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct reboot_mode_driver *reboot;
>> +	struct mode_info *info;
>> +	ssize_t size = 0;
>> +
>> +	reboot = container_of(dev, struct reboot_mode_driver, reboot_mode_device);
>> +	if (!reboot)
>> +		return -ENODATA;
>> +
>> +	list_for_each_entry(info, &reboot->head, list)
>> +		size += sysfs_emit_at(buf, size, "%s ", info->mode);
>> +
> 
> Don't you need locking here? What if we'll end up adding a new reboot mode at
> the same time?

reboot-modes are added only once the time of reboot_mode_register_device. Yes,
there can be a race here if someone read while module load or probe was in progress.
Sure we can add a lock here to protect against the addition in reboot_mode_register_device.

> 
>> +	if (!size)
>> +		return -ENODATA;
>> +
>> +	return size + sysfs_emit_at(buf, size - 1, "\n");
>> +}
>> +static DEVICE_ATTR_RO(reboot_modes);
>> +
>> +static struct attribute *reboot_mode_attrs[] = {
>> +	&dev_attr_reboot_modes.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(reboot_mode);
>> +
>> +static const struct class reboot_mode_class = {
>> +	.name = "reboot-mode",
>> +	.dev_groups = reboot_mode_groups,
>> +};
>> +
>> +static void reboot_mode_device_release(struct device *dev)
>> +{
>> +    /* place holder to avoid warning on device_unregister. nothing to free */
>> +}
>> +
>> +static void reboot_mode_register_device(struct reboot_mode_driver *reboot)
>> +{
>> +	reboot->reboot_mode_device.class = &reboot_mode_class;
>> +	reboot->reboot_mode_device.release = reboot_mode_device_release;
>> +	dev_set_name(&reboot->reboot_mode_device, reboot->driver_name);
>> +	if (!device_register(&reboot->reboot_mode_device))
>> +		reboot->reboot_mode_device_registered = true;
>> +	else
>> +		reboot->reboot_mode_device_registered = false;
> 
> Just use device_create(). I would also suggest creating a private structure
> that embeds the pointer to the struct device created by device_create() and
> the pointer to the reboot_mode_driver. If you pass it as driver data to
> device_create(), you'll be able to retrieve it with dev_get_drvdata() in
> sysfs callbacks.

Had made change to use device_create and dev_get_drvdata in below change, and have then
changed it to above as per the reviews on the same.
https://lore.kernel.org/all/qhlxxfsyc42xemerhi36myvil3bf45isgmpugkuqzsvgcc3ifn@njrtwuooij2q/

Should we change to device_create?

> 
>> +}
>> +
>>  static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>>  					  const char *cmd)
>>  {
>> @@ -78,6 +127,9 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>
>>  	INIT_LIST_HEAD(&reboot->head);
>>
>> +	if (reboot_mode_class_registered)
>> +		reboot_mode_register_device(reboot);
>> +
>>  	for_each_property_of_node(np, prop) {
>>  		if (strncmp(prop->name, PREFIX, len))
>>  			continue;
>> @@ -119,6 +171,11 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>  	list_for_each_entry(info, &reboot->head, list)
>>  		kfree_const(info->mode);
>>
>> +	if (reboot->reboot_mode_device_registered) {
>> +		device_unregister(&reboot->reboot_mode_device);
>> +		reboot->reboot_mode_device_registered = false;
>> +	}
>> +
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(reboot_mode_register);
>> @@ -136,6 +193,11 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>>  	list_for_each_entry(info, &reboot->head, list)
>>  		kfree_const(info->mode);
>>
>> +	if (reboot->reboot_mode_device_registered) {
>> +		device_unregister(&reboot->reboot_mode_device);
>> +		reboot->reboot_mode_device_registered = false;
>> +	}
>> +
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(reboot_mode_unregister);
>> @@ -162,6 +224,7 @@ int devm_reboot_mode_register(struct device *dev,
>>  	if (!dr)
>>  		return -ENOMEM;
>>
>> +	reboot->driver_name = reboot->dev->driver->name;
>>  	rc = reboot_mode_register(reboot);
>>  	if (rc) {
>>  		devres_free(dr);
>> @@ -199,6 +262,29 @@ void devm_reboot_mode_unregister(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(devm_reboot_mode_unregister);
>>
>> +static int __init reboot_mode_init(void)
>> +{
>> +	if (class_register(&reboot_mode_class))
>> +		reboot_mode_class_registered = false;
>> +	else
>> +		reboot_mode_class_registered = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit reboot_mode_exit(void)
>> +{
>> +	if (reboot_mode_class_registered)
>> +		class_unregister(&reboot_mode_class);
> 
> Please take a moment to read the code of the functions you're calling. The
> class_unregister() function will check if the class you're passing as argument
> was previously registered. If not, it will do nothing. You don't need all this
> checking.

Ack. thanks.

> 
>> +}
>> +
>> +#ifdef MODULE
>> +module_init(reboot_mode_init);
>> +module_exit(reboot_mode_exit);
>> +#else
>> +subsys_initcall(reboot_mode_init);
>> +#endif
> 
> You don't need to do this. It's perfectly fine to just do:
> 
> subsys_initcall(reboot_mode_init);
> module_exit(reboot_mode_exit);
> 
> This is handled in the module.h header[1].

Ack. sure. Will remove it.

> 
>> +
>>  MODULE_AUTHOR("Andy Yan <andy.yan@...k-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..400cfde0e029aef14ff90a11b9d12d0c3ce8dee6 100644
>> --- a/include/linux/reboot-mode.h
>> +++ b/include/linux/reboot-mode.h
>> @@ -5,6 +5,9 @@
>>  struct reboot_mode_driver {
>>  	struct device *dev;
>>  	struct list_head head;
>> +	const char *driver_name;
>> +	struct device reboot_mode_device;
>> +	bool reboot_mode_device_registered;
> 
> No need for this, just use device_is_registered() wherever you need to check
> it.

Ack.

thanks,
Shivendra

Powered by blists - more mailing lists