Message-ID: <a45f1d23-c224-ba7d-eddc-a622cefd2ba0@oss.qualcomm.com>
Date: Wed, 12 Nov 2025 22:54:15 +0530
From: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
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 11/10/2025 9:45 PM, Bjorn Andersson wrote:
> 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.

sure. Will split this series based on dependencies.

the psci patch does has a dependency on the fwnode based registration
and the u64 bit magic registration. Let me see how can i split the series
to 2 independent patchsets. Any suggestions will be helpful.

potentially it may be.
1 - devres removal + expose sysfs
2 - u64 bit registration = fw node + any extras + psci. 

> 
>> 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?

Ack. will update this.

> 
>> +
>>  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.

Ack. will update.

> 
>> +{
>> +	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);
> 

Ack. thanks.

>> +	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.
> 

Ack. will update this.

>> +	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.
>

sure i can modify this to continue on 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;

Ack.

thanks,
Shivendra

Powered by blists - more mailing lists