Message-ID: <qhlxxfsyc42xemerhi36myvil3bf45isgmpugkuqzsvgcc3ifn@njrtwuooij2q>
Date: Mon, 10 Nov 2025 10:15:32 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, 
	Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>, 
	Sudeep Holla <sudeep.holla@....com>, Souvik Chakravarty <Souvik.Chakravarty@....com>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Andy Yan <andy.yan@...k-chips.com>, Mark Rutland <mark.rutland@....com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Arnd Bergmann <arnd@...db.de>, 
	Konrad Dybcio <konradybcio@...nel.org>, cros-qcom-dts-watchers@...omium.org, 
	Vinod Koul <vkoul@...nel.org>, Catalin Marinas <catalin.marinas@....com>, 
	Will Deacon <will@...nel.org>, Florian Fainelli <florian.fainelli@...adcom.com>, 
	Moritz Fischer <moritz.fischer@...us.com>, John Stultz <john.stultz@...aro.org>, 
	Matthias Brugger <matthias.bgg@...il.com>, Krzysztof Kozlowski <krzk@...nel.org>, 
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, Mukesh Ojha <mukesh.ojha@....qualcomm.com>, 
	Stephen Boyd <swboyd@...omium.org>, Andre Draszik <andre.draszik@...aro.org>, 
	Kathiravan Thirumoorthy <kathiravan.thirumoorthy@....qualcomm.com>, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-arm-msm@...r.kernel.org, Elliot Berman <quic_eberman@...cinc.com>, 
	Xin Liu <xin.liu@....qualcomm.com>, Srinivas Kandagatla <srini@...nel.org>
Subject: Re: [PATCH v17 05/12] power: reset: reboot-mode: Expose sysfs for
 registered reboot_modes

On Sun, Nov 09, 2025 at 08:07:18PM +0530, Shivendra Pratap 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.
> 

I like this addition, and your commit message reasoning about this
addition. But, while touching upon the same subject, you've made this
series add two separate things.

So now this part can't be merged unless there's agreement on the PSCI
SYSTEM_RESET2, and the PSCI SYSTEM_RESET2 can't be merged unless this
sysfs interface is agreed upon.

Unless I'm missing some clear dependency here, it would have been better
to keep these two topics in separate series, and drive them to
conclusion independently.

> Signed-off-by: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
> ---
>  drivers/power/reset/reboot-mode.c | 62 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/reboot-mode.h       |  2 ++
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index 873ac45cd7659b214b7c21958f580ca381e0a63d..582aa7f8ed7fa485c5a67877558c9b15d3600ef4 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -6,6 +6,7 @@
>  #define pr_fmt(fmt)	"reboot-mode: " fmt
>  
>  #include <linux/device.h>
> +#include <linux/err.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -23,6 +24,8 @@ struct mode_info {
>  	struct list_head list;
>  };
>  
> +static struct class *rb_class;

Why not "static const struct class reboot_mode_class" and then a
class_register() call? Why do you need the class dynamically allocated
on the heap?

> +
>  static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
>  {
>  	const char *normal = "normal";
> @@ -65,6 +68,51 @@ static int reboot_mode_notify(struct notifier_block *this,
>  	return NOTIFY_DONE;
>  }
>  
> +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 = (struct reboot_mode_driver *)dev_get_drvdata(dev);
> +	if (!reboot)
> +		return -ENODATA;
> +
> +	list_for_each_entry(info, &reboot->head, list)
> +		size += sysfs_emit_at(buf, size, "%s ", info->mode);
> +
> +	if (size) {
> +		size += sysfs_emit_at(buf, size - 1, "\n");
> +		return size;
> +	}
> +
> +	return -ENODATA;
> +}
> +static DEVICE_ATTR_RO(reboot_modes);
> +
> +static int create_reboot_mode_device(struct reboot_mode_driver *reboot)

Note how (almost) all other function names in this file start with
a "reboot_mode_" prefix.

> +{
> +	int ret = 0;

First use is an assignment, no need for you to zero-initialize it here.

> +
> +	if (!rb_class) {
> +		rb_class = class_create("reboot-mode");
> +		if (IS_ERR(rb_class))
> +			return PTR_ERR(rb_class);
> +	}
> +
> +	reboot->reboot_dev = device_create(rb_class, NULL, 0, (void *)reboot, reboot->driver_name);

Every struct reboot_mode_driver is going to end up having one of these,
so why not incorporate it into the reboot_mode_driver in the first
place. It avoids the extra heap allocation, and you can use
container_of() instead of drv_data to find your reboot_mode_driver in
the reboot_modes_show() above.


Just:
  reboot->reboot_dev.class = &reboot_mode_class;
  dev_set_name(&reboot->reboot_dev, reboot->driver_name);
  ret = device_register(&reboot->reboot_dev);

> +	if (IS_ERR(reboot->reboot_dev))
> +		return PTR_ERR(reboot->reboot_dev);
> +
> +	ret = device_create_file(reboot->reboot_dev, &dev_attr_reboot_modes);

Manually creating sysfs attributes is both error prone and racy, so if
you can you should avoid it.

Here you have the opportunity to just statically assign
reboot_mode_class->dev_groups to an ATTRIBUTE_GROUP() with your
attribute and it will all be handled for you.

> +	if (ret) {
> +		device_unregister(reboot->reboot_dev);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * reboot_mode_register - register a reboot mode driver
>   * @reboot: reboot mode driver
> @@ -83,13 +131,17 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>  	u32 magic_arg2;
>  	int ret;
>  
> -	if (!fwnode)
> +	if (!fwnode || !reboot->driver_name)
>  		return -EINVAL;
>  
>  	np = to_of_node(fwnode);
>  	if (!np)
>  		return -EINVAL;
>  
> +	ret = create_reboot_mode_device(reboot);
> +	if (ret)
> +		return ret;
> +
>  	INIT_LIST_HEAD(&reboot->head);
>  
>  	for_each_property_of_node(np, prop) {
> @@ -142,6 +194,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>  		kfree(info);
>  	}
>  
> +	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
> +	device_unregister(reboot->reboot_dev);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(reboot_mode_register);
> @@ -155,6 +209,9 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>  	struct mode_info *info;
>  	struct mode_info *next;
>  
> +	if (!reboot->reboot_dev)
> +		return -EINVAL;
> +
>  	unregister_reboot_notifier(&reboot->reboot_notifier);
>  
>  	list_for_each_entry_safe(info, next, &reboot->head, list) {
> @@ -163,6 +220,8 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>  		kfree(info);
>  	}
>  
> +	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
> +	device_unregister(reboot->reboot_dev);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(reboot_mode_unregister);
> @@ -192,6 +251,7 @@ int devm_reboot_mode_register(struct device *dev,
>  	if (!dr)
>  		return -ENOMEM;
>  
> +	reboot->driver_name = reboot->dev->driver->name;

It seems unlikely that we will have multiple instances of the same
driver influencing the actual reboot mode, but we could very well have
multiple instances of the same driver calling
devm_reboot_mode_register(). E.g. on a board two PMICs, both with PON
blocks (but only one considered as the source for boot mode).

In that case you will end up trying to create multiple devices with the
name "qcom-pon", presumably that will fail and per your error handling
you have now disabled the reboot-mechanism for all but the first pon
instance that was registered.

It also creates some asymmetry between devm_reboot_mode_register() and
reboot_mode_register(), in that the one API the client driver decides
the name, in other it's hard coded to the driver name (and if the client
did specify a name - which they should if they use the non-devm one- it
will be overwritten).



On that note, I would argue that aborting the registration of
reboot-modes, just because we failed to create the convenient "debug"
interface, doesn't make sense. I think it would be better to just
continue even when create_reboot_mode_device() returns an error.

>  	rc = reboot_mode_register(reboot, of_fwnode_handle(reboot->dev->of_node));
>  	if (rc) {
>  		devres_free(dr);
> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
> index e0d3e8a54050a76f26846f456120b4c7e371d284..81c149edf40fbcf0d3427c2e12eb415199cb153b 100644
> --- a/include/linux/reboot-mode.h
> +++ b/include/linux/reboot-mode.h
> @@ -7,6 +7,8 @@
>  
>  struct reboot_mode_driver {
>  	struct device *dev;
> +	struct device *reboot_dev;

As suggested above:

struct device reboot_dev;

Regards,
Bjorn

> +	const char *driver_name;
>  	struct list_head head;
>  	int (*write)(struct reboot_mode_driver *reboot, u64 magic);
>  	struct notifier_block reboot_notifier;
> 
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists