[PATCH v2] reboot: allow to specify reboot mode via sysfs

[PATCH v2] reboot: allow to specify reboot mode via sysfs

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

The kernel cmdline reboot= option offers some sort of control
on how the reboot is issued.
Add handles in sysfs to allow setting these reboot options, so they
can be changed when the system is booted, other than at boot time.

The handlers are under <sysfs>/kernel/reboot, can be read to
get the current configuration and written to alter it.

	# cd /sys/kernel/reboot/

	# grep . *
	cpu:0
	force:0
	mode:cold
	type:acpi

	# echo 2 >cpu
	# echo 1 >force
	# echo soft >mode
	# echo bios >type

	# grep . *
	cpu:2
	force:1
	mode:soft
	type:bios

Before setting anything, check for CAP_SYS_BOOT capability, so it's
possible to allow an unpriviledged process to change these settings
simply by relaxing the handles permissions, without opening them to
the world.

Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
 Documentation/ABI/testing/sysfs-kernel-reboot |  26 +++
 kernel/reboot.c                               | 193 ++++++++++++++++++
 2 files changed, 219 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-reboot

diff --git a/Documentation/ABI/testing/sysfs-kernel-reboot b/Documentation/ABI/testing/sysfs-kernel-reboot
new file mode 100644
index 000000000000..3fda90bdc644
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-reboot
@@ -0,0 +1,26 @@
+What:		/sys/kernel/reboot
+Date:		October 2020
+KernelVersion:	5.11
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	Interface to set the kernel reboot mode, similarly to
+		what can be done via the reboot= cmdline option.
+		(see Documentation/admin-guide/kernel-parameters.txt)
+
+What:		/sys/kernel/reboot/mode
+What:		/sys/kernel/reboot/type
+What:		/sys/kernel/reboot/cpu
+What:		/sys/kernel/reboot/force
+
+Date:		October 2020
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	Tune reboot parameters.
+
+		mode: Reboot mode. Valid values are:
+		cold warm hard soft gpio
+
+		type: Reboot type. Valid values are:
+		bios acpi kbd triple efi pci
+
+		cpu: CPU number to use to reboot.
+
+		force: Force an immediate reboot.
diff --git a/kernel/reboot.c b/kernel/reboot.c
index e7b78d5ae1ab..b9e607517ae3 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -594,3 +594,196 @@ static int __init reboot_setup(char *str)
 	return 1;
 }
 __setup("reboot=", reboot_setup);
+
+#ifdef CONFIG_SYSFS
+
+#define STARTS_WITH(s, sc) (!strncmp(s, sc, sizeof(sc)-1))
+
+static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	const char *val;
+
+	switch (reboot_mode) {
+	case REBOOT_COLD:
+		val = "cold\n";
+		break;
+	case REBOOT_WARM:
+		val = "warm\n";
+		break;
+	case REBOOT_HARD:
+		val = "hard\n";
+		break;
+	case REBOOT_SOFT:
+		val = "soft\n";
+		break;
+	case REBOOT_GPIO:
+		val = "gpio\n";
+		break;
+	default:
+		val = "undefined\n";
+	}
+
+	return strscpy(buf, val, 10);
+}
+static ssize_t mode_store(struct kobject *kobj, struct kobj_attribute *attr,
+			  const char *buf, size_t count)
+{
+	if (!capable(CAP_SYS_BOOT))
+		return -EPERM;
+
+	if (STARTS_WITH(buf, "cold"))
+		reboot_mode = REBOOT_COLD;
+	else if (STARTS_WITH(buf, "warm"))
+		reboot_mode = REBOOT_WARM;
+	else if (STARTS_WITH(buf, "hard"))
+		reboot_mode = REBOOT_HARD;
+	else if (STARTS_WITH(buf, "soft"))
+		reboot_mode = REBOOT_SOFT;
+	else if (STARTS_WITH(buf, "gpio"))
+		reboot_mode = REBOOT_GPIO;
+	else
+		return -EINVAL;
+
+	return count;
+}
+static struct kobj_attribute reboot_mode_attr = __ATTR_RW(mode);
+
+static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	const char *val;
+
+	switch (reboot_type) {
+	case BOOT_TRIPLE:
+		val = "triple\n";
+		break;
+	case BOOT_KBD:
+		val = "kbd\n";
+		break;
+	case BOOT_BIOS:
+		val = "bios\n";
+		break;
+	case BOOT_ACPI:
+		val = "acpi\n";
+		break;
+	case BOOT_EFI:
+		val = "efi\n";
+		break;
+	case BOOT_CF9_FORCE:
+		val = "cf9_force\n";
+		break;
+	case BOOT_CF9_SAFE:
+		val = "cf9_safe\n";
+		break;
+	default:
+		val = "undefined\n";
+	}
+
+	return strscpy(buf, val, 10);
+}
+static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
+			  const char *buf, size_t count)
+{
+	if (!capable(CAP_SYS_BOOT))
+		return -EPERM;
+
+	if (STARTS_WITH(buf, "triple"))
+		reboot_type = BOOT_TRIPLE;
+	else if (STARTS_WITH(buf, "kbd"))
+		reboot_type = BOOT_KBD;
+	else if (STARTS_WITH(buf, "bios"))
+		reboot_type = BOOT_BIOS;
+	else if (STARTS_WITH(buf, "acpi"))
+		reboot_type = BOOT_ACPI;
+	else if (STARTS_WITH(buf, "efi"))
+		reboot_type = BOOT_EFI;
+	else if (STARTS_WITH(buf, "cf9_force"))
+		reboot_type = BOOT_CF9_FORCE;
+	else if (STARTS_WITH(buf, "cf9_safe"))
+		reboot_type = BOOT_CF9_SAFE;
+	else
+		return -EINVAL;
+
+	return count;
+}
+static struct kobj_attribute reboot_type_attr = __ATTR_RW(type);
+
+#undef STARTS_WITH
+
+static ssize_t cpu_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", reboot_cpu);
+}
+static ssize_t cpu_store(struct kobject *kobj, struct kobj_attribute *attr,
+			  const char *buf, size_t count)
+{
+	unsigned int cpunum;
+	int rc;
+
+	if (!capable(CAP_SYS_BOOT))
+		return -EPERM;
+
+	rc = kstrtouint(buf, 0, &cpunum);
+
+	if (rc)
+		return rc;
+
+	if (cpunum >= num_possible_cpus())
+		return -ERANGE;
+
+	reboot_cpu = cpunum;
+
+	return count;
+}
+static struct kobj_attribute reboot_cpu_attr = __ATTR_RW(cpu);
+
+static ssize_t force_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", reboot_force);
+}
+static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
+			  const char *buf, size_t count)
+{
+	if (!capable(CAP_SYS_BOOT))
+		return -EPERM;
+
+	if (buf[0] != '0' && buf[0] != '1')
+		return -EINVAL;
+
+	reboot_force = buf[0] - '0';
+
+	return count;
+}
+static struct kobj_attribute reboot_force_attr = __ATTR_RW(force);
+
+static struct attribute *reboot_attrs[] = {
+	&reboot_mode_attr.attr,
+	&reboot_type_attr.attr,
+	&reboot_cpu_attr.attr,
+	&reboot_force_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group reboot_attr_group = {
+	.attrs = reboot_attrs,
+};
+
+static int __init reboot_ksysfs_init(void)
+{
+	struct kobject *reboot_kobj;
+	int ret;
+
+	reboot_kobj = kobject_create_and_add("reboot", kernel_kobj);
+	if (!reboot_kobj)
+		return -ENOMEM;
+
+	ret = sysfs_create_group(reboot_kobj, &reboot_attr_group);
+	if (ret) {
+		kobject_put(reboot_kobj);
+		return ret;
+	}
+
+	return 0;
+}
+core_initcall(reboot_ksysfs_init);
+
+#endif
-- 
2.28.0

Re: [PATCH v2] reboot: allow to specify reboot mode via sysfs

[Andrew Morton]

On Fri,  6 Nov 2020 21:07:04 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:

> The kernel cmdline reboot= option offers some sort of control
> on how the reboot is issued.
> Add handles in sysfs to allow setting these reboot options, so they
> can be changed when the system is booted, other than at boot time.

Please include a description of why you believe the kernel needs this
feature.  Use cases, end-user benefits, etc.

We've survived this long without it - what changed?

Re: [PATCH v2] reboot: allow to specify reboot mode via sysfs

[Matteo Croce]

On Fri, Nov 6, 2020 at 11:33 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri,  6 Nov 2020 21:07:04 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> > The kernel cmdline reboot= option offers some sort of control
> > on how the reboot is issued.
> > Add handles in sysfs to allow setting these reboot options, so they
> > can be changed when the system is booted, other than at boot time.
>
> Please include a description of why you believe the kernel needs this
> feature.  Use cases, end-user benefits, etc.
>
> We've survived this long without it - what changed?

Hi Andrew,

We don't always know in advance what type of reboot to perform.

Sometimes a warm reboot is preferred to persist certain memory regions
across the reboot. Others a cold one is needed to apply a future system
update that makes a memory memory model change, like changing the base
page size or resizing a persistent memory region.

Or simply we want to enable reboot_force because we noticed that
something bad happened.

Bye,
-- 
per aspera ad upstream

Re: [PATCH v2] reboot: allow to specify reboot mode via sysfs

[Petr Mladek]

On Fri 2020-11-06 21:07:04, Matteo Croce wrote:
> From: Matteo Croce <mcroce@microsoft.com>
> 
> The kernel cmdline reboot= option offers some sort of control
> on how the reboot is issued.
> Add handles in sysfs to allow setting these reboot options, so they
> can be changed when the system is booted, other than at boot time.
> 
> The handlers are under <sysfs>/kernel/reboot, can be read to
> get the current configuration and written to alter it.
> 
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-kernel-reboot
> @@ -0,0 +1,26 @@
> +What:		/sys/kernel/reboot
> +Date:		October 2020
> +KernelVersion:	5.11
> +Contact:	Matteo Croce <mcroce@microsoft.com>
> +Description:	Interface to set the kernel reboot mode, similarly to
> +		what can be done via the reboot= cmdline option.
> +		(see Documentation/admin-guide/kernel-parameters.txt)
> +
> +What:		/sys/kernel/reboot/mode
> +What:		/sys/kernel/reboot/type
> +What:		/sys/kernel/reboot/cpu
> +What:		/sys/kernel/reboot/force

I do not see any file where it is accumulated this way.
It seems that each path is always described separately.

I am not sure if it is really needed. But it might be needed
when processing the API documentation.

Please, split it.


> +
> +Date:		October 2020
> +Contact:	Matteo Croce <mcroce@microsoft.com>
> +Description:	Tune reboot parameters.
> +
> +		mode: Reboot mode. Valid values are:
> +		cold warm hard soft gpio
> +
> +		type: Reboot type. Valid values are:
> +		bios acpi kbd triple efi pci
> +
> +		cpu: CPU number to use to reboot.
> +
> +		force: Force an immediate reboot.
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index e7b78d5ae1ab..b9e607517ae3 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -594,3 +594,196 @@ static int __init reboot_setup(char *str)
>  	return 1;
>  }
>  __setup("reboot=", reboot_setup);
> +
> +#ifdef CONFIG_SYSFS
> +
> +#define STARTS_WITH(s, sc) (!strncmp(s, sc, sizeof(sc)-1))
> +
> +static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	const char *val;
> +
> +	switch (reboot_mode) {
> +	case REBOOT_COLD:
> +		val = "cold\n";

Using "\n" everywhere is weird. Also the same strings are
repeated in the next functions.

I suggest to define them only once, e.g.

#define REBOOT_COLD_STR "cold"
#define REBOOT_WARM_STR "warm"

and use here:

	case REBOOT_COLD:
		val = REBOOT_COLD_STR;

and then at the end

	return sprintf(buf, "%s\n", val);


> +		break;
> +	case REBOOT_WARM:
> +		val = "warm\n";
> +		break;
> +	case REBOOT_HARD:
> +		val = "hard\n";
> +		break;
> +	case REBOOT_SOFT:
> +		val = "soft\n";
> +		break;
> +	case REBOOT_GPIO:
> +		val = "gpio\n";
> +		break;
> +	default:
> +		val = "undefined\n";
> +	}
> +
> +	return strscpy(buf, val, 10);

"undefined\n" needs 11 bytes to store also the trailing '\0'.
Anyway, the buffer should be big enough for all variants.


> +}
> +static ssize_t mode_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	if (!capable(CAP_SYS_BOOT))
> +		return -EPERM;
> +
> +	if (STARTS_WITH(buf, "cold"))
> +		reboot_mode = REBOOT_COLD;

I would prefer to open code this and use strlen(). It will be obvious
what the code does immediately. And I am sure that compilators
will optimize out the strlen().


	if (strncmp(buf, REBOOT_COLD_STR, strlen(REBOOT_COLD_STR)) == 0)
		reboot_mode = REBOOT_COLD;
	else if (strncmp(buf, REBOOT_WARM_STR, strlen(REBOOT_WARM_STR) == 0)
		reboot_mode = REBOOT_WARM;
	...



> +	else if (STARTS_WITH(buf, "warm"))
> +		reboot_mode = REBOOT_WARM;
> +	else if (STARTS_WITH(buf, "hard"))
> +		reboot_mode = REBOOT_HARD;
> +	else if (STARTS_WITH(buf, "soft"))
> +		reboot_mode = REBOOT_SOFT;
> +	else if (STARTS_WITH(buf, "gpio"))
> +		reboot_mode = REBOOT_GPIO;
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +static struct kobj_attribute reboot_mode_attr = __ATTR_RW(mode);
> +
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	const char *val;
> +
> +	switch (reboot_type) {
> +	case BOOT_TRIPLE:
> +		val = "triple\n";

Same here

		var = BOOT_TRIPLE_STR;

> +		break;
> +	case BOOT_KBD:
> +		val = "kbd\n";
> +		break;
> +	case BOOT_BIOS:
> +		val = "bios\n";
> +		break;
> +	case BOOT_ACPI:
> +		val = "acpi\n";
> +		break;
> +	case BOOT_EFI:
> +		val = "efi\n";
> +		break;
> +	case BOOT_CF9_FORCE:
> +		val = "cf9_force\n";
> +		break;
> +	case BOOT_CF9_SAFE:
> +		val = "cf9_safe\n";
> +		break;
> +	default:
> +		val = "undefined\n";
> +	}
> +
> +	return strscpy(buf, val, 10);
> +}
> +static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	if (!capable(CAP_SYS_BOOT))
> +		return -EPERM;
> +
> +	if (STARTS_WITH(buf, "triple"))
> +		reboot_type = BOOT_TRIPLE;

and here:

	if (strncmp(buf, REBOOT_TRIPLE_STR, strlen(REBOOT_TRIPLE_STR)) == 0)
		reboot_type = REBOOT_TRIPLE;


> +	else if (STARTS_WITH(buf, "kbd"))
> +		reboot_type = BOOT_KBD;
> +	else if (STARTS_WITH(buf, "bios"))
> +		reboot_type = BOOT_BIOS;
> +	else if (STARTS_WITH(buf, "acpi"))
> +		reboot_type = BOOT_ACPI;
> +	else if (STARTS_WITH(buf, "efi"))
> +		reboot_type = BOOT_EFI;
> +	else if (STARTS_WITH(buf, "cf9_force"))
> +		reboot_type = BOOT_CF9_FORCE;
> +	else if (STARTS_WITH(buf, "cf9_safe"))
> +		reboot_type = BOOT_CF9_SAFE;
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +static struct kobj_attribute reboot_type_attr = __ATTR_RW(type);
> +
> +static ssize_t force_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", reboot_force);
> +}
> +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	if (!capable(CAP_SYS_BOOT))
> +		return -EPERM;
> +
> +	if (buf[0] != '0' && buf[0] != '1')
> +		return -EINVAL;

Please use kstrtobool() that supports also other boolean values,
for example, 'Y', 'n'.

> +	rc = kstrtouint(buf, 0, &cpunum);
> +
> +	reboot_force = buf[0] - '0';
> +
> +	return count;
> +}

> +static int __init reboot_ksysfs_init(void)
> +{
> +	struct kobject *reboot_kobj;
> +	int ret;
> +
> +	reboot_kobj = kobject_create_and_add("reboot", kernel_kobj);
> +	if (!reboot_kobj)
> +		return -ENOMEM;
> +
> +	ret = sysfs_create_group(reboot_kobj, &reboot_attr_group);
> +	if (ret) {
> +		kobject_put(reboot_kobj);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +core_initcall(reboot_ksysfs_init);

There is no need to create the sysfs interface this early. In fact, it
might even break because the parent "kernel" node is defined
as core_initcall() as well. The order is not defined in this case.

I would do it as sybsys_initcall() like or even late_initcall().

Best Regards,
Petr

Re: [PATCH v2] reboot: allow to specify reboot mode via sysfs

[Matteo Croce]

On Mon, Nov 9, 2020 at 3:16 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2020-11-06 21:07:04, Matteo Croce wrote:
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > The kernel cmdline reboot= option offers some sort of control
> > on how the reboot is issued.
> > Add handles in sysfs to allow setting these reboot options, so they
> > can be changed when the system is booted, other than at boot time.
> >
> > The handlers are under <sysfs>/kernel/reboot, can be read to
> > get the current configuration and written to alter it.
> >
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-kernel-reboot
> > @@ -0,0 +1,26 @@
> > +What:                /sys/kernel/reboot
> > +Date:                October 2020
> > +KernelVersion:       5.11
> > +Contact:     Matteo Croce <mcroce@microsoft.com>
> > +Description: Interface to set the kernel reboot mode, similarly to
> > +             what can be done via the reboot= cmdline option.
> > +             (see Documentation/admin-guide/kernel-parameters.txt)
> > +
> > +What:                /sys/kernel/reboot/mode
> > +What:                /sys/kernel/reboot/type
> > +What:                /sys/kernel/reboot/cpu
> > +What:                /sys/kernel/reboot/force
>
> I do not see any file where it is accumulated this way.
> It seems that each path is always described separately.
>

I've found it in Documentation/ABI/testing/sysfs-kernel-mm-ksm


> I am not sure if it is really needed. But it might be needed
> when processing the API documentation.
>
> Please, split it.
>

Ok

>
> > +
> > +Date:                October 2020
> > +Contact:     Matteo Croce <mcroce@microsoft.com>
> > +Description: Tune reboot parameters.
> > +
> > +             mode: Reboot mode. Valid values are:
> > +             cold warm hard soft gpio
> > +
> > +             type: Reboot type. Valid values are:
> > +             bios acpi kbd triple efi pci
> > +
> > +             cpu: CPU number to use to reboot.
> > +
> > +             force: Force an immediate reboot.
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index e7b78d5ae1ab..b9e607517ae3 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -594,3 +594,196 @@ static int __init reboot_setup(char *str)
> >       return 1;
> >  }
> >  __setup("reboot=", reboot_setup);
> > +
> > +#ifdef CONFIG_SYSFS
> > +
> > +#define STARTS_WITH(s, sc) (!strncmp(s, sc, sizeof(sc)-1))
> > +
> > +static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     const char *val;
> > +
> > +     switch (reboot_mode) {
> > +     case REBOOT_COLD:
> > +             val = "cold\n";
>
> Using "\n" everywhere is weird. Also the same strings are
> repeated in the next functions.
>
> I suggest to define them only once, e.g.
>
> #define REBOOT_COLD_STR "cold"
> #define REBOOT_WARM_STR "warm"
>
> and use here:
>
>         case REBOOT_COLD:
>                 val = REBOOT_COLD_STR;
>
> and then at the end
>
>         return sprintf(buf, "%s\n", val);
>
>

Ok.

> > +             break;
> > +     case REBOOT_WARM:
> > +             val = "warm\n";
> > +             break;
> > +     case REBOOT_HARD:
> > +             val = "hard\n";
> > +             break;
> > +     case REBOOT_SOFT:
> > +             val = "soft\n";
> > +             break;
> > +     case REBOOT_GPIO:
> > +             val = "gpio\n";
> > +             break;
> > +     default:
> > +             val = "undefined\n";
> > +     }
> > +
> > +     return strscpy(buf, val, 10);
>
> "undefined\n" needs 11 bytes to store also the trailing '\0'.
> Anyway, the buffer should be big enough for all variants.
>
>

Oops, missed it.

> > +}
> > +static ssize_t mode_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +                       const char *buf, size_t count)
> > +{
> > +     if (!capable(CAP_SYS_BOOT))
> > +             return -EPERM;
> > +
> > +     if (STARTS_WITH(buf, "cold"))
> > +             reboot_mode = REBOOT_COLD;
>
> I would prefer to open code this and use strlen(). It will be obvious
> what the code does immediately. And I am sure that compilators
> will optimize out the strlen().
>
>
>         if (strncmp(buf, REBOOT_COLD_STR, strlen(REBOOT_COLD_STR)) == 0)
>                 reboot_mode = REBOOT_COLD;
>         else if (strncmp(buf, REBOOT_WARM_STR, strlen(REBOOT_WARM_STR) == 0)
>                 reboot_mode = REBOOT_WARM;
>         ...
>

Yes, since they are constant.

>
>
> > +     else if (STARTS_WITH(buf, "warm"))
> > +             reboot_mode = REBOOT_WARM;
> > +     else if (STARTS_WITH(buf, "hard"))
> > +             reboot_mode = REBOOT_HARD;
> > +     else if (STARTS_WITH(buf, "soft"))
> > +             reboot_mode = REBOOT_SOFT;
> > +     else if (STARTS_WITH(buf, "gpio"))
> > +             reboot_mode = REBOOT_GPIO;
> > +     else
> > +             return -EINVAL;
> > +
> > +     return count;
> > +}
> > +static struct kobj_attribute reboot_mode_attr = __ATTR_RW(mode);
> > +
> > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     const char *val;
> > +
> > +     switch (reboot_type) {
> > +     case BOOT_TRIPLE:
> > +             val = "triple\n";
>
> Same here
>
>                 var = BOOT_TRIPLE_STR;
>
> > +             break;
> > +     case BOOT_KBD:
> > +             val = "kbd\n";
> > +             break;
> > +     case BOOT_BIOS:
> > +             val = "bios\n";
> > +             break;
> > +     case BOOT_ACPI:
> > +             val = "acpi\n";
> > +             break;
> > +     case BOOT_EFI:
> > +             val = "efi\n";
> > +             break;
> > +     case BOOT_CF9_FORCE:
> > +             val = "cf9_force\n";
> > +             break;
> > +     case BOOT_CF9_SAFE:
> > +             val = "cf9_safe\n";
> > +             break;
> > +     default:
> > +             val = "undefined\n";
> > +     }
> > +
> > +     return strscpy(buf, val, 10);
> > +}
> > +static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +                       const char *buf, size_t count)
> > +{
> > +     if (!capable(CAP_SYS_BOOT))
> > +             return -EPERM;
> > +
> > +     if (STARTS_WITH(buf, "triple"))
> > +             reboot_type = BOOT_TRIPLE;
>
> and here:
>
>         if (strncmp(buf, REBOOT_TRIPLE_STR, strlen(REBOOT_TRIPLE_STR)) == 0)
>                 reboot_type = REBOOT_TRIPLE;
>
>
> > +     else if (STARTS_WITH(buf, "kbd"))
> > +             reboot_type = BOOT_KBD;
> > +     else if (STARTS_WITH(buf, "bios"))
> > +             reboot_type = BOOT_BIOS;
> > +     else if (STARTS_WITH(buf, "acpi"))
> > +             reboot_type = BOOT_ACPI;
> > +     else if (STARTS_WITH(buf, "efi"))
> > +             reboot_type = BOOT_EFI;
> > +     else if (STARTS_WITH(buf, "cf9_force"))
> > +             reboot_type = BOOT_CF9_FORCE;
> > +     else if (STARTS_WITH(buf, "cf9_safe"))
> > +             reboot_type = BOOT_CF9_SAFE;
> > +     else
> > +             return -EINVAL;
> > +
> > +     return count;
> > +}
> > +static struct kobj_attribute reboot_type_attr = __ATTR_RW(type);
> > +
> > +static ssize_t force_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     return sprintf(buf, "%d\n", reboot_force);
> > +}
> > +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +                       const char *buf, size_t count)
> > +{
> > +     if (!capable(CAP_SYS_BOOT))
> > +             return -EPERM;
> > +
> > +     if (buf[0] != '0' && buf[0] != '1')
> > +             return -EINVAL;
>
> Please use kstrtobool() that supports also other boolean values,
> for example, 'Y', 'n'.
>

Nice, will do.

> > +     rc = kstrtouint(buf, 0, &cpunum);
> > +
> > +     reboot_force = buf[0] - '0';
> > +
> > +     return count;
> > +}
>
> > +static int __init reboot_ksysfs_init(void)
> > +{
> > +     struct kobject *reboot_kobj;
> > +     int ret;
> > +
> > +     reboot_kobj = kobject_create_and_add("reboot", kernel_kobj);
> > +     if (!reboot_kobj)
> > +             return -ENOMEM;
> > +
> > +     ret = sysfs_create_group(reboot_kobj, &reboot_attr_group);
> > +     if (ret) {
> > +             kobject_put(reboot_kobj);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +core_initcall(reboot_ksysfs_init);
>
> There is no need to create the sysfs interface this early. In fact, it
> might even break because the parent "kernel" node is defined
> as core_initcall() as well. The order is not defined in this case.
>
> I would do it as sybsys_initcall() like or even late_initcall().
>

I'll postpone it to late_initcall().

Thanks,
-- 
per aspera ad upstream