linux-kernel - Re: [PATCH v20 2/2] power: reset: reboot-mode: Expose sysfs for registered reboot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4f8f9217-f6d1-1321-c2be-5ee1dd807eca@oss.qualcomm.com>
Date: Mon, 1 Dec 2025 23:13:04 +0530
From: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
To: Bartosz Golaszewski <brgl@...nel.org>
Cc: Sebastian Reichel <sre@...nel.org>,
        Bartosz Golaszewski <bgolasze@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v20 2/2] power: reset: reboot-mode: Expose sysfs for
 registered reboot_modes



On 12/1/2025 6:51 PM, Bartosz Golaszewski wrote:
> On Sun, Nov 30, 2025 at 7:21 PM Shivendra Pratap
> <shivendra.pratap@....qualcomm.com> wrote:
>>

[SNIP..]
>>
>> +static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +       struct reboot_mode_driver *reboot;
> 
> This is not related to this patch but please consider proposing
> renaming of this structure to struct reboot_mode_devicd because
> calling it a driver is quite confusing where in reality it's a device.
> 
>> +       struct mode_info *info;
>> +       ssize_t size = 0;
>> +
>> +       reboot = container_of(dev, struct reboot_mode_driver, reboot_mode_device);
>> +       if (!reboot)
>> +               return -ENODATA;
> 
> container_of(p, t, m) returns the address of the structure of type t
> containing the member pointed to by p known under the name m in the
> type of t. It just calculates the offset between the address of m in t
> and p. It's not possible for it to return NULL.

Ack. Will remove this check. thanks.

> 
>> +       scoped_guard(mutex, &reboot->reboot_mode_mutex) {
>> +               list_for_each_entry(info, &reboot->head, list)
>> +                       size += sysfs_emit_at(buf, size, "%s ", info->mode);
>> +       }
>> +
>> +       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)
>> +{
>> +       int ret;
>> +
>> +       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);
>> +       /* Check return value to avoid compiler warning */
>> +       ret = device_register(&reboot->reboot_mode_device);
>> +       if (ret)
>> +               pr_debug("device_register failed for %s : %d\n", reboot->driver_name, ret);
>> +}
> 
> I'm not sure if this has been addressed before but why is this even
> optional? Why don't we just return -1 here if we fail to register the
> device?

Sure. we can return -1 and then continue to register reboot-modes.

As per reviews on old thread, we wanted to continue reboot-mode
registration, even if creation of reboot_mode_device fails.
https://lore.kernel.org/all/qhlxxfsyc42xemerhi36myvil3bf45isgmpugkuqzsvgcc3ifn@njrtwuooij2q/
 
> 
> And just for the record: I'm still convinced that using
> device_create() here will result in nicer code and allow us to contain
> the associated reboot_mode_device inside this compilation unit.
> 
> If Bjorn really insists on keeping it this way though, then you need
> at least a call to device_initialize() here.
> 
>> +
>>  static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>>                                           const char *cmd)
>>  {
>> @@ -76,6 +128,7 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>         size_t len = strlen(PREFIX);
>>         int ret;
>>
>> +       mutex_init(&reboot->reboot_mode_mutex);
>>         INIT_LIST_HEAD(&reboot->head);
>>
>>         for_each_property_of_node(np, prop) {
>> @@ -112,6 +165,7 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>
>>         reboot->reboot_notifier.notifier_call = reboot_mode_notify;
>>         register_reboot_notifier(&reboot->reboot_notifier);
>> +       reboot_mode_register_device(reboot);
>>
>>         return 0;
>>
>> @@ -132,9 +186,13 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>>         struct mode_info *info;
>>
>>         unregister_reboot_notifier(&reboot->reboot_notifier);
>> +       if (device_is_registered(&reboot->reboot_mode_device))
>> +               device_unregister(&reboot->reboot_mode_device);
> 
> If you bail out of reboot_mode_register_device(), you don't need the
> above check anymore because the device could

We wanted to continue on failure, as per reviews.

thanks,
Shivendra