linux-kernel - Re: [PATCH v21 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] [day] [month] [year] [list]

Message-ID: <CAMRc=MeBiHcYd_3p9k=QOc5zxC930W6=aaD4Jbh9zhMWjwZ=bA@mail.gmail.com>
Date: Mon, 22 Dec 2025 18:26:10 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
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 v21 2/2] power: reset: reboot-mode: Expose sysfs for
 registered reboot_modes

On Mon, Dec 22, 2025 at 4:33 AM Shivendra Pratap
<shivendra.pratap@....qualcomm.com> 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.
>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
> ---

[snip]

> +
>  static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>                                           const char *cmd)
>  {
> @@ -76,6 +109,15 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>         size_t len = strlen(PREFIX);
>         int ret;
>
> +       reboot->reboot_mode_device = device_create(&reboot_mode_class, NULL, 0,
> +                                                  (void *)reboot, reboot->driver_name);

You should define a separate struct in this file and pass it as
drvdata as argument 4. The main reason for using device_create() was
to not have to store any data associated with the sysfs ABI in a
separate struct, not in the public one.

> +       if (IS_ERR(reboot->reboot_mode_device)) {
> +               ret = PTR_ERR(reboot->reboot_mode_device);
> +               reboot->reboot_mode_device = NULL;
> +               return ret;
> +       }
> +
> +       mutex_init(&reboot->reboot_mode_mutex);

Add a corresponding mutex_destroy() please.

[snip]

> +
>  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..d7141a1a609b62bd3185642ecc1478fdd3555037 100644
> --- a/include/linux/reboot-mode.h
> +++ b/include/linux/reboot-mode.h
> @@ -2,9 +2,15 @@
>  #ifndef __REBOOT_MODE_H__
>  #define __REBOOT_MODE_H__
>
> +#include <linux/mutex.h>
> +
>  struct reboot_mode_driver {
>         struct device *dev;
>         struct list_head head;
> +       const char *driver_name;

I have no idea why you're storing the name here.

As I said above: you should not need to modify this structure (if
maybe for the mutex if modifications of this struct from existing code
can race with the sysfs code). Use a separate one for sysfs data.

> +       struct device *reboot_mode_device;

I think you misunderstood my comment about the renaming: what I meant
was: propose to rename the existing reboot_mode_driver structure to
reboot_mode_device because this is what it is in reality.

> +       /* protects reboot_mode list */
> +       struct mutex reboot_mode_mutex;
>         int (*write)(struct reboot_mode_driver *reboot, unsigned int magic);
>         struct notifier_block reboot_notifier;
>  };
>
> --
> 2.34.1
>

Bart