linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] reboot: allow to specify reboot mode via sysfs
@ 2020-11-10 20:27 Matteo Croce
  2020-11-12  3:50 ` [PATCH] reboot: Fix variable assignments in type_store Nathan Chancellor
  2021-02-22 10:03 ` [PATCH v4] reboot: allow to specify reboot mode via sysfs Pavel Machek
  0 siblings, 2 replies; 19+ messages in thread
From: Matteo Croce @ 2020-11-10 20:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mike Rapoport, Guenter Roeck, Arnd Bergmann, Petr Mladek,
	Pavel Tatashin, Kees Cook, Andrew Morton, Tyler Hicks

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 yes >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 |  32 +++
 kernel/reboot.c                               | 206 ++++++++++++++++++
 2 files changed, 238 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..837330fb2511
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-reboot
@@ -0,0 +1,32 @@
+What:		/sys/kernel/reboot
+Date:		November 2020
+KernelVersion:	5.11
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	Interface to set the kernel reboot behavior, similarly to
+		what can be done via the reboot= cmdline option.
+		(see Documentation/admin-guide/kernel-parameters.txt)
+
+What:		/sys/kernel/reboot/mode
+Date:		November 2020
+KernelVersion:	5.11
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	Reboot mode. Valid values are: cold warm hard soft gpio
+
+What:		/sys/kernel/reboot/type
+Date:		November 2020
+KernelVersion:	5.11
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	Reboot type. Valid values are: bios acpi kbd triple efi pci
+
+What:		/sys/kernel/reboot/cpu
+Date:		November 2020
+KernelVersion:	5.11
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	CPU number to use to reboot.
+
+What:		/sys/kernel/reboot/force
+Date:		November 2020
+KernelVersion:	5.11
+Contact:	Matteo Croce <mcroce@microsoft.com>
+Description:	Don't wait for any other CPUs on reboot and
+		avoid anything that could hang.
diff --git a/kernel/reboot.c b/kernel/reboot.c
index e7b78d5ae1ab..81cc0f0594c6 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -594,3 +594,209 @@ static int __init reboot_setup(char *str)
 	return 1;
 }
 __setup("reboot=", reboot_setup);
+
+#ifdef CONFIG_SYSFS
+
+#define REBOOT_COLD_STR		"cold"
+#define REBOOT_WARM_STR		"warm"
+#define REBOOT_HARD_STR		"hard"
+#define REBOOT_SOFT_STR		"soft"
+#define REBOOT_GPIO_STR		"gpio"
+#define REBOOT_UNDEFINED_STR	"undefined"
+
+#define BOOT_TRIPLE_STR		"triple"
+#define BOOT_KBD_STR		"kbd"
+#define BOOT_BIOS_STR		"bios"
+#define BOOT_ACPI_STR		"acpi"
+#define BOOT_EFI_STR		"efi"
+#define BOOT_CF9_FORCE_STR	"cf9_force"
+#define BOOT_CF9_SAFE_STR	"cf9_safe"
+
+static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	const char *val;
+
+	switch (reboot_mode) {
+	case REBOOT_COLD:
+		val = REBOOT_COLD_STR;
+		break;
+	case REBOOT_WARM:
+		val = REBOOT_WARM_STR;
+		break;
+	case REBOOT_HARD:
+		val = REBOOT_HARD_STR;
+		break;
+	case REBOOT_SOFT:
+		val = REBOOT_SOFT_STR;
+		break;
+	case REBOOT_GPIO:
+		val = REBOOT_GPIO_STR;
+		break;
+	default:
+		val = REBOOT_UNDEFINED_STR;
+	}
+
+	return sprintf(buf, "%s\n", val);
+}
+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 (!strncmp(buf, REBOOT_COLD_STR, strlen(REBOOT_COLD_STR)))
+		reboot_mode = REBOOT_COLD;
+	else if (!strncmp(buf, REBOOT_WARM_STR, strlen(REBOOT_WARM_STR)))
+		reboot_mode = REBOOT_WARM;
+	else if (!strncmp(buf, REBOOT_HARD_STR, strlen(REBOOT_HARD_STR)))
+		reboot_mode = REBOOT_HARD;
+	else if (!strncmp(buf, REBOOT_SOFT_STR, strlen(REBOOT_SOFT_STR)))
+		reboot_mode = REBOOT_SOFT;
+	else if (!strncmp(buf, REBOOT_GPIO_STR, strlen(REBOOT_GPIO_STR)))
+		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 = BOOT_TRIPLE_STR;
+		break;
+	case BOOT_KBD:
+		val = BOOT_KBD_STR;
+		break;
+	case BOOT_BIOS:
+		val = BOOT_BIOS_STR;
+		break;
+	case BOOT_ACPI:
+		val = BOOT_ACPI_STR;
+		break;
+	case BOOT_EFI:
+		val = BOOT_EFI_STR;
+		break;
+	case BOOT_CF9_FORCE:
+		val = BOOT_CF9_FORCE_STR;
+		break;
+	case BOOT_CF9_SAFE:
+		val = BOOT_CF9_SAFE_STR;
+		break;
+	default:
+		val = REBOOT_UNDEFINED_STR;
+	}
+
+	return sprintf(buf, "%s\n", val);
+}
+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 (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
+		reboot_mode = BOOT_TRIPLE;
+	else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
+		reboot_mode = BOOT_KBD;
+	else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
+		reboot_mode = BOOT_BIOS;
+	else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
+		reboot_mode = BOOT_ACPI;
+	else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
+		reboot_mode = BOOT_EFI;
+	else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
+		reboot_mode = BOOT_CF9_FORCE;
+	else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
+		reboot_mode = BOOT_CF9_SAFE;
+	else
+		return -EINVAL;
+
+	return count;
+}
+static struct kobj_attribute reboot_type_attr = __ATTR_RW(type);
+
+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)
+{
+	bool res;
+
+	if (!capable(CAP_SYS_BOOT))
+		return -EPERM;
+
+	if (kstrtobool(buf, &res))
+		return -EINVAL;
+
+	reboot_force = res;
+
+	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;
+}
+late_initcall(reboot_ksysfs_init);
+
+#endif
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] reboot: Fix variable assignments in type_store
  2020-11-10 20:27 [PATCH v4] reboot: allow to specify reboot mode via sysfs Matteo Croce
@ 2020-11-12  3:50 ` Nathan Chancellor
  2020-11-12 11:26   ` Matteo Croce
                     ` (2 more replies)
  2021-02-22 10:03 ` [PATCH v4] reboot: allow to specify reboot mode via sysfs Pavel Machek
  1 sibling, 3 replies; 19+ messages in thread
From: Nathan Chancellor @ 2020-11-12  3:50 UTC (permalink / raw)
  To: mcroce
  Cc: akpm, arnd, keescook, linux-kernel, linux, pasha.tatashin,
	pmladek, rppt, tyhicks, ndesaulniers, clang-built-linux,
	Nathan Chancellor

Clang warns:

kernel/reboot.c:707:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_TRIPLE;
                            ~ ^~~~~~~~~~~
kernel/reboot.c:709:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_KBD;
                            ~ ^~~~~~~~
kernel/reboot.c:711:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_BIOS;
                            ~ ^~~~~~~~~
kernel/reboot.c:713:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_ACPI;
                            ~ ^~~~~~~~~
kernel/reboot.c:715:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_EFI;
                            ~ ^~~~~~~~
kernel/reboot.c:717:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_CF9_FORCE;
                            ~ ^~~~~~~~~~~~~~
kernel/reboot.c:719:17: warning: implicit conversion from enumeration
type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
[-Wenum-conversion]
                reboot_mode = BOOT_CF9_SAFE;
                            ~ ^~~~~~~~~~~~~
7 warnings generated.

It seems that these assignment should be to reboot_type, not
reboot_mode. Fix it so there are no more warnings and the code works
properly.

Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
Link: https://github.com/ClangBuiltLinux/linux/issues/1197
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 kernel/reboot.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index deba133a071b..8599d0d44aec 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
 		return -EPERM;
 
 	if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
-		reboot_mode = BOOT_TRIPLE;
+		reboot_type = BOOT_TRIPLE;
 	else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
-		reboot_mode = BOOT_KBD;
+		reboot_type = BOOT_KBD;
 	else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
-		reboot_mode = BOOT_BIOS;
+		reboot_type = BOOT_BIOS;
 	else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
-		reboot_mode = BOOT_ACPI;
+		reboot_type = BOOT_ACPI;
 	else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
-		reboot_mode = BOOT_EFI;
+		reboot_type = BOOT_EFI;
 	else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
-		reboot_mode = BOOT_CF9_FORCE;
+		reboot_type = BOOT_CF9_FORCE;
 	else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
-		reboot_mode = BOOT_CF9_SAFE;
+		reboot_type = BOOT_CF9_SAFE;
 	else
 		return -EINVAL;
 

base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12  3:50 ` [PATCH] reboot: Fix variable assignments in type_store Nathan Chancellor
@ 2020-11-12 11:26   ` Matteo Croce
  2020-11-12 17:49     ` Nathan Chancellor
  2020-11-12 23:13   ` Andrew Morton
  2020-11-13 15:56   ` Petr Mladek
  2 siblings, 1 reply; 19+ messages in thread
From: Matteo Croce @ 2020-11-12 11:26 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Andrew Morton, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, ndesaulniers, clang-built-linux

On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_TRIPLE;
>                             ~ ^~~~~~~~~~~
> kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_KBD;
>                             ~ ^~~~~~~~
> kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_BIOS;
>                             ~ ^~~~~~~~~
> kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_ACPI;
>                             ~ ^~~~~~~~~
> kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_EFI;
>                             ~ ^~~~~~~~
> kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_CF9_FORCE;
>                             ~ ^~~~~~~~~~~~~~
> kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_CF9_SAFE;
>                             ~ ^~~~~~~~~~~~~
> 7 warnings generated.
>
> It seems that these assignment should be to reboot_type, not
> reboot_mode. Fix it so there are no more warnings and the code works
> properly.
>
> Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  kernel/reboot.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index deba133a071b..8599d0d44aec 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
>                 return -EPERM;
>
>         if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> -               reboot_mode = BOOT_TRIPLE;
> +               reboot_type = BOOT_TRIPLE;
>         else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> -               reboot_mode = BOOT_KBD;
> +               reboot_type = BOOT_KBD;
>         else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> -               reboot_mode = BOOT_BIOS;
> +               reboot_type = BOOT_BIOS;
>         else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> -               reboot_mode = BOOT_ACPI;
> +               reboot_type = BOOT_ACPI;
>         else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> -               reboot_mode = BOOT_EFI;
> +               reboot_type = BOOT_EFI;
>         else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> -               reboot_mode = BOOT_CF9_FORCE;
> +               reboot_type = BOOT_CF9_FORCE;
>         else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> -               reboot_mode = BOOT_CF9_SAFE;
> +               reboot_type = BOOT_CF9_SAFE;
>         else
>                 return -EINVAL;
>
>
> base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> --
> 2.29.2
>

Hmm, this was introduced in v3 I think.

I wonder why my compiler doesn't warn about it, the two variables are
defined as different enum type.
I get the same warnings with GCC and -Wenum-conversion.

Thanks,
--
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12 11:26   ` Matteo Croce
@ 2020-11-12 17:49     ` Nathan Chancellor
  2020-11-12 17:59       ` Matteo Croce
  0 siblings, 1 reply; 19+ messages in thread
From: Nathan Chancellor @ 2020-11-12 17:49 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Andrew Morton, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, ndesaulniers, clang-built-linux

Hi Matteo,

On Thu, Nov 12, 2020 at 12:26:45PM +0100, Matteo Croce wrote:
> On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns:
> >
> > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_TRIPLE;
> >                             ~ ^~~~~~~~~~~
> > kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_KBD;
> >                             ~ ^~~~~~~~
> > kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_BIOS;
> >                             ~ ^~~~~~~~~
> > kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_ACPI;
> >                             ~ ^~~~~~~~~
> > kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_EFI;
> >                             ~ ^~~~~~~~
> > kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_CF9_FORCE;
> >                             ~ ^~~~~~~~~~~~~~
> > kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_CF9_SAFE;
> >                             ~ ^~~~~~~~~~~~~
> > 7 warnings generated.
> >
> > It seems that these assignment should be to reboot_type, not
> > reboot_mode. Fix it so there are no more warnings and the code works
> > properly.
> >
> > Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  kernel/reboot.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index deba133a071b..8599d0d44aec 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> >                 return -EPERM;
> >
> >         if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > -               reboot_mode = BOOT_TRIPLE;
> > +               reboot_type = BOOT_TRIPLE;
> >         else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > -               reboot_mode = BOOT_KBD;
> > +               reboot_type = BOOT_KBD;
> >         else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > -               reboot_mode = BOOT_BIOS;
> > +               reboot_type = BOOT_BIOS;
> >         else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > -               reboot_mode = BOOT_ACPI;
> > +               reboot_type = BOOT_ACPI;
> >         else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > -               reboot_mode = BOOT_EFI;
> > +               reboot_type = BOOT_EFI;
> >         else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> > -               reboot_mode = BOOT_CF9_FORCE;
> > +               reboot_type = BOOT_CF9_FORCE;
> >         else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> > -               reboot_mode = BOOT_CF9_SAFE;
> > +               reboot_type = BOOT_CF9_SAFE;
> >         else
> >                 return -EINVAL;
> >
> >
> > base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> > --
> > 2.29.2
> >
> 
> Hmm, this was introduced in v3 I think.
> 
> I wonder why my compiler doesn't warn about it, the two variables are
> defined as different enum type.
> I get the same warnings with GCC and -Wenum-conversion.

What version of GCC do you have? -Wenum-conversion is a fairly new
warning in GCC I think. Although if you get it now, maybe it was some
configuration error?

Regardless, thank you for taking a look at the patch!

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12 17:49     ` Nathan Chancellor
@ 2020-11-12 17:59       ` Matteo Croce
  2020-11-12 18:09         ` Nathan Chancellor
  0 siblings, 1 reply; 19+ messages in thread
From: Matteo Croce @ 2020-11-12 17:59 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Andrew Morton, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Thu, Nov 12, 2020 at 6:49 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Hi Matteo,
>
> On Thu, Nov 12, 2020 at 12:26:45PM +0100, Matteo Croce wrote:
> > On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns:
> > >
> > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_TRIPLE;
> > >                             ~ ^~~~~~~~~~~
> > > kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_KBD;
> > >                             ~ ^~~~~~~~
> > > kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_BIOS;
> > >                             ~ ^~~~~~~~~
> > > kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_ACPI;
> > >                             ~ ^~~~~~~~~
> > > kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_EFI;
> > >                             ~ ^~~~~~~~
> > > kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_CF9_FORCE;
> > >                             ~ ^~~~~~~~~~~~~~
> > > kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_CF9_SAFE;
> > >                             ~ ^~~~~~~~~~~~~
> > > 7 warnings generated.
> > >
> > > It seems that these assignment should be to reboot_type, not
> > > reboot_mode. Fix it so there are no more warnings and the code works
> > > properly.
> > >
> > > Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  kernel/reboot.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > > index deba133a071b..8599d0d44aec 100644
> > > --- a/kernel/reboot.c
> > > +++ b/kernel/reboot.c
> > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> > >                 return -EPERM;
> > >
> > >         if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > > -               reboot_mode = BOOT_TRIPLE;
> > > +               reboot_type = BOOT_TRIPLE;
> > >         else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > > -               reboot_mode = BOOT_KBD;
> > > +               reboot_type = BOOT_KBD;
> > >         else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > > -               reboot_mode = BOOT_BIOS;
> > > +               reboot_type = BOOT_BIOS;
> > >         else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > > -               reboot_mode = BOOT_ACPI;
> > > +               reboot_type = BOOT_ACPI;
> > >         else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > > -               reboot_mode = BOOT_EFI;
> > > +               reboot_type = BOOT_EFI;
> > >         else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> > > -               reboot_mode = BOOT_CF9_FORCE;
> > > +               reboot_type = BOOT_CF9_FORCE;
> > >         else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> > > -               reboot_mode = BOOT_CF9_SAFE;
> > > +               reboot_type = BOOT_CF9_SAFE;
> > >         else
> > >                 return -EINVAL;
> > >
> > >
> > > base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> > > --
> > > 2.29.2
> > >
> >
> > Hmm, this was introduced in v3 I think.
> >
> > I wonder why my compiler doesn't warn about it, the two variables are
> > defined as different enum type.
> > I get the same warnings with GCC and -Wenum-conversion.
>
> What version of GCC do you have? -Wenum-conversion is a fairly new
> warning in GCC I think. Although if you get it now, maybe it was some
> configuration error?
>

Hi,

the one shipped in Fedora 33:
gcc version 10.2.1 20201016 (Red Hat 10.2.1-6) (GCC)

I enabled -Wenum-compare -Wenum-conversion globally in the root
Makefile and I had only 15 warnings for an 'allyesconfig' x86_64
build.

Maybe it's worth fixing them and enable the warning, it's very useful.

Thanks,
-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12 17:59       ` Matteo Croce
@ 2020-11-12 18:09         ` Nathan Chancellor
  0 siblings, 0 replies; 19+ messages in thread
From: Nathan Chancellor @ 2020-11-12 18:09 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Andrew Morton, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Thu, Nov 12, 2020 at 06:59:36PM +0100, Matteo Croce wrote:
> On Thu, Nov 12, 2020 at 6:49 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Hi Matteo,
> >
> > On Thu, Nov 12, 2020 at 12:26:45PM +0100, Matteo Croce wrote:
> > > On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > >
> > > > Clang warns:
> > > >
> > > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_TRIPLE;
> > > >                             ~ ^~~~~~~~~~~
> > > > kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_KBD;
> > > >                             ~ ^~~~~~~~
> > > > kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_BIOS;
> > > >                             ~ ^~~~~~~~~
> > > > kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_ACPI;
> > > >                             ~ ^~~~~~~~~
> > > > kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_EFI;
> > > >                             ~ ^~~~~~~~
> > > > kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_CF9_FORCE;
> > > >                             ~ ^~~~~~~~~~~~~~
> > > > kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > >                 reboot_mode = BOOT_CF9_SAFE;
> > > >                             ~ ^~~~~~~~~~~~~
> > > > 7 warnings generated.
> > > >
> > > > It seems that these assignment should be to reboot_type, not
> > > > reboot_mode. Fix it so there are no more warnings and the code works
> > > > properly.
> > > >
> > > > Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > ---
> > > >  kernel/reboot.c | 14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > > > index deba133a071b..8599d0d44aec 100644
> > > > --- a/kernel/reboot.c
> > > > +++ b/kernel/reboot.c
> > > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > >                 return -EPERM;
> > > >
> > > >         if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > > > -               reboot_mode = BOOT_TRIPLE;
> > > > +               reboot_type = BOOT_TRIPLE;
> > > >         else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > > > -               reboot_mode = BOOT_KBD;
> > > > +               reboot_type = BOOT_KBD;
> > > >         else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > > > -               reboot_mode = BOOT_BIOS;
> > > > +               reboot_type = BOOT_BIOS;
> > > >         else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > > > -               reboot_mode = BOOT_ACPI;
> > > > +               reboot_type = BOOT_ACPI;
> > > >         else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > > > -               reboot_mode = BOOT_EFI;
> > > > +               reboot_type = BOOT_EFI;
> > > >         else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> > > > -               reboot_mode = BOOT_CF9_FORCE;
> > > > +               reboot_type = BOOT_CF9_FORCE;
> > > >         else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> > > > -               reboot_mode = BOOT_CF9_SAFE;
> > > > +               reboot_type = BOOT_CF9_SAFE;
> > > >         else
> > > >                 return -EINVAL;
> > > >
> > > >
> > > > base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> > > > --
> > > > 2.29.2
> > > >
> > >
> > > Hmm, this was introduced in v3 I think.
> > >
> > > I wonder why my compiler doesn't warn about it, the two variables are
> > > defined as different enum type.
> > > I get the same warnings with GCC and -Wenum-conversion.
> >
> > What version of GCC do you have? -Wenum-conversion is a fairly new
> > warning in GCC I think. Although if you get it now, maybe it was some
> > configuration error?
> >
> 
> Hi,
> 
> the one shipped in Fedora 33:
> gcc version 10.2.1 20201016 (Red Hat 10.2.1-6) (GCC)
> 
> I enabled -Wenum-compare -Wenum-conversion globally in the root
> Makefile and I had only 15 warnings for an 'allyesconfig' x86_64
> build.
> 
> Maybe it's worth fixing them and enable the warning, it's very useful.

Ahh, I thought that -Wenum-conversion was enabled with -Wall for GCC, it
appears to be enabled by -Wextra. I believe that Arnd has been tracking
down these instances with GCC and sending patches, I assume in hopes of
turning on the warning globally. I agree, the warning is definitely
helpful and clang has helped catch several bugs with it.

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12  3:50 ` [PATCH] reboot: Fix variable assignments in type_store Nathan Chancellor
  2020-11-12 11:26   ` Matteo Croce
@ 2020-11-12 23:13   ` Andrew Morton
  2020-11-13  0:20     ` Matteo Croce
  2020-11-13 15:56   ` Petr Mladek
  2 siblings, 1 reply; 19+ messages in thread
From: Andrew Morton @ 2020-11-12 23:13 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: mcroce, arnd, keescook, linux-kernel, linux, pasha.tatashin,
	pmladek, rppt, tyhicks, ndesaulniers, clang-built-linux

On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:

> Clang warns:
> 
> kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_TRIPLE;
>                             ~ ^~~~~~~~~~~
>
> ...
>
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		return -EPERM;
>  
>  	if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> -		reboot_mode = BOOT_TRIPLE;
> +		reboot_type = BOOT_TRIPLE;
>  	else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> -		reboot_mode = BOOT_KBD;
> +		reboot_type = BOOT_KBD;
>  	else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> -		reboot_mode = BOOT_BIOS;
> +		reboot_type = BOOT_BIOS;
>  	else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> -		reboot_mode = BOOT_ACPI;
> +		reboot_type = BOOT_ACPI;
>  	else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> -		reboot_mode = BOOT_EFI;
> +		reboot_type = BOOT_EFI;
>  	else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> -		reboot_mode = BOOT_CF9_FORCE;
> +		reboot_type = BOOT_CF9_FORCE;
>  	else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> -		reboot_mode = BOOT_CF9_SAFE;
> +		reboot_type = BOOT_CF9_SAFE;
>  	else
>  		return -EINVAL;

This is a fairly dramatic change to the original patch, but it eyeballs
OK.

Matteo, could you please comment?  And preferably retest?


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12 23:13   ` Andrew Morton
@ 2020-11-13  0:20     ` Matteo Croce
  2020-11-13  0:41       ` Matteo Croce
  2020-11-13  1:18       ` Andrew Morton
  0 siblings, 2 replies; 19+ messages in thread
From: Matteo Croce @ 2020-11-13  0:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Nathan Chancellor, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, Nov 13, 2020 at 12:13 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:
>
> > Clang warns:
> >
> > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> >                 reboot_mode = BOOT_TRIPLE;
> >                             ~ ^~~~~~~~~~~
> >
> > ...
> >
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> >               return -EPERM;
> >
> >       if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > -             reboot_mode = BOOT_TRIPLE;
> > +             reboot_type = BOOT_TRIPLE;
> >       else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > -             reboot_mode = BOOT_KBD;
> > +             reboot_type = BOOT_KBD;
> >       else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > -             reboot_mode = BOOT_BIOS;
> > +             reboot_type = BOOT_BIOS;
> >       else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > -             reboot_mode = BOOT_ACPI;
> > +             reboot_type = BOOT_ACPI;
> >       else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > -             reboot_mode = BOOT_EFI;
> > +             reboot_type = BOOT_EFI;
> >       else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> > -             reboot_mode = BOOT_CF9_FORCE;
> > +             reboot_type = BOOT_CF9_FORCE;
> >       else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> > -             reboot_mode = BOOT_CF9_SAFE;
> > +             reboot_type = BOOT_CF9_SAFE;
> >       else
> >               return -EINVAL;
>
> This is a fairly dramatic change to the original patch, but it eyeballs
> OK.
>
> Matteo, could you please comment?  And preferably retest?
>

Hi,

I reviewed the patch and it looks good to me.
I tested it with this script which passes now with Nathan's fix:

for i in cold warm hard soft gpio; do
    echo $i > mode
    read j <mode
    [ $i = $j ] || echo "mode $i = $j"
done

for i in bios acpi kbd triple efi cf9_force cf9_safe; do
    echo $i > type
    read j <type
    [ $i = $j ] || echo "type $i = $j"
done

for i in $(seq 0 $(nproc --ignore=1)); do
    echo $i > cpu
    read j <cpu
    [ $i = $j ] || echo "cpu $i = $j"
done

for i in 0 1; do
    echo $i >force
    read j <force
    [ $i = $j ] || echo "force $i = $j"
done

While writing the script I found that in the documentation I left for
'type' the values from
Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for
cf9_force reboot.
While at it, should we update the doc with the values 'cf9_force' and
'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the
kernel cmdline?

In any case, kernel-parameters.txt doesn't mention that reboot=q does
the 'cf9_safe' reboot type, so it must be fixed anyway.

Regards,
-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13  0:20     ` Matteo Croce
@ 2020-11-13  0:41       ` Matteo Croce
  2020-11-13  1:18       ` Andrew Morton
  1 sibling, 0 replies; 19+ messages in thread
From: Matteo Croce @ 2020-11-13  0:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Nathan Chancellor, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, Nov 13, 2020 at 1:20 AM Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> On Fri, Nov 13, 2020 at 12:13 AM Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:
> >
> > > Clang warns:
> > >
> > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > >                 reboot_mode = BOOT_TRIPLE;
> > >                             ~ ^~~~~~~~~~~
> > >
> > > ...
> > >
> > > --- a/kernel/reboot.c
> > > +++ b/kernel/reboot.c
> > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> > >               return -EPERM;
> > >
> > >       if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > > -             reboot_mode = BOOT_TRIPLE;
> > > +             reboot_type = BOOT_TRIPLE;
> > >       else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > > -             reboot_mode = BOOT_KBD;
> > > +             reboot_type = BOOT_KBD;
> > >       else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > > -             reboot_mode = BOOT_BIOS;
> > > +             reboot_type = BOOT_BIOS;
> > >       else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > > -             reboot_mode = BOOT_ACPI;
> > > +             reboot_type = BOOT_ACPI;
> > >       else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > > -             reboot_mode = BOOT_EFI;
> > > +             reboot_type = BOOT_EFI;
> > >       else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> > > -             reboot_mode = BOOT_CF9_FORCE;
> > > +             reboot_type = BOOT_CF9_FORCE;
> > >       else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> > > -             reboot_mode = BOOT_CF9_SAFE;
> > > +             reboot_type = BOOT_CF9_SAFE;
> > >       else
> > >               return -EINVAL;
> >
> > This is a fairly dramatic change to the original patch, but it eyeballs
> > OK.
> >
> > Matteo, could you please comment?  And preferably retest?
> >
>
> Hi,
>
> I reviewed the patch and it looks good to me.
> I tested it with this script which passes now with Nathan's fix:
>
> for i in cold warm hard soft gpio; do
>     echo $i > mode
>     read j <mode
>     [ $i = $j ] || echo "mode $i = $j"
> done
>
> for i in bios acpi kbd triple efi cf9_force cf9_safe; do
>     echo $i > type
>     read j <type
>     [ $i = $j ] || echo "type $i = $j"
> done
>
> for i in $(seq 0 $(nproc --ignore=1)); do
>     echo $i > cpu
>     read j <cpu
>     [ $i = $j ] || echo "cpu $i = $j"
> done
>
> for i in 0 1; do
>     echo $i >force
>     read j <force
>     [ $i = $j ] || echo "force $i = $j"
> done
>
> While writing the script I found that in the documentation I left for
> 'type' the values from
> Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for
> cf9_force reboot.
> While at it, should we update the doc with the values 'cf9_force' and
> 'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the
> kernel cmdline?
>
> In any case, kernel-parameters.txt doesn't mention that reboot=q does
> the 'cf9_safe' reboot type, so it must be fixed anyway.
>
> Regards,
> --
> per aspera ad upstream

Reviewed-and-tested-by: Matteo Croce <mcroce@microsoft.com>

-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13  0:20     ` Matteo Croce
  2020-11-13  0:41       ` Matteo Croce
@ 2020-11-13  1:18       ` Andrew Morton
  2020-11-13  1:38         ` Matteo Croce
  1 sibling, 1 reply; 19+ messages in thread
From: Andrew Morton @ 2020-11-13  1:18 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Nathan Chancellor, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, 13 Nov 2020 01:20:29 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:

> While writing the script I found that in the documentation I left for
> 'type' the values from
> Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for
> cf9_force reboot.
> While at it, should we update the doc with the values 'cf9_force' and
> 'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the
> kernel cmdline?

I looked at Documentation/admin-guide/kernel-parameters.txt's "reboot="
section and decided that I don't understand your above words :( Can you
please expand on all of this?  Simple akpm-compatible words ;)

> In any case, kernel-parameters.txt doesn't mention that reboot=q does
> the 'cf9_safe' reboot type, so it must be fixed anyway.

Thanks for noticing.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13  1:18       ` Andrew Morton
@ 2020-11-13  1:38         ` Matteo Croce
  2020-11-13  2:46           ` Andrew Morton
  0 siblings, 1 reply; 19+ messages in thread
From: Matteo Croce @ 2020-11-13  1:38 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Nathan Chancellor, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, Nov 13, 2020 at 2:18 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Nov 2020 01:20:29 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> > While writing the script I found that in the documentation I left for
> > 'type' the values from
> > Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for
> > cf9_force reboot.
> > While at it, should we update the doc with the values 'cf9_force' and
> > 'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the
> > kernel cmdline?
>
> I looked at Documentation/admin-guide/kernel-parameters.txt's "reboot="
> section and decided that I don't understand your above words :( Can you
> please expand on all of this?  Simple akpm-compatible words ;)
>

Heh sorry :)

I misread the code, I thought that BOOT_CF9_SAFE was user selectable
because of the enum value:

enum reboot_type {
...
BOOT_CF9_FORCE = 'p',
BOOT_CF9_SAFE = 'q',
};

But when parsing the cmdline, 'q' is simply ignored, so it's just an
internal flag.
It's used only by arch/x86/kernel/reboot.c in the loop which tries to
reboot in different modes until it succeeds.

The doc is right, never mind.

At this point, since 'pci' enables BOOT_CF9_FORCE type and
BOOT_CF9_SAFE is not user selectable, should I simply leave only
'pci'?
This way, we'll have the same set of options for both sysfs and kernel cmdline.

-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13  1:38         ` Matteo Croce
@ 2020-11-13  2:46           ` Andrew Morton
  2020-11-13  2:58             ` Matteo Croce
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Morton @ 2020-11-13  2:46 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Nathan Chancellor, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:

> At this point, since 'pci' enables BOOT_CF9_FORCE type and
> BOOT_CF9_SAFE is not user selectable, should I simply leave only
> 'pci'?
> This way, we'll have the same set of options for both sysfs and kernel cmdline.

Well, you're the reboot expert ;)

But my $0.02 is yes, let's keep the command-line and sysfs interfaces
in sync and cover it all in documentation.  It would of course be
problematic to change the existing reboot= interface.

I assume that means doing this?

- #define BOOT_CF9_FORCE_STR     "cf9_force"
+ #define BOOT_CF9_FORCE_STR     "pci"
- #define BOOT_CF9_SAFE_STR      "cf9_safe"

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13  2:46           ` Andrew Morton
@ 2020-11-13  2:58             ` Matteo Croce
  2020-11-13 20:06               ` Petr Mladek
  0 siblings, 1 reply; 19+ messages in thread
From: Matteo Croce @ 2020-11-13  2:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Nathan Chancellor, Arnd Bergmann, Kees Cook, linux-kernel,
	Guenter Roeck, Pavel Tatashin, Petr Mladek, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> > At this point, since 'pci' enables BOOT_CF9_FORCE type and
> > BOOT_CF9_SAFE is not user selectable, should I simply leave only
> > 'pci'?
> > This way, we'll have the same set of options for both sysfs and kernel cmdline.
>
> Well, you're the reboot expert ;)
>

So honored! :)

> But my $0.02 is yes, let's keep the command-line and sysfs interfaces
> in sync and cover it all in documentation.  It would of course be
> problematic to change the existing reboot= interface.
>
> I assume that means doing this?
>
> - #define BOOT_CF9_FORCE_STR     "cf9_force"
> + #define BOOT_CF9_FORCE_STR     "pci"
> - #define BOOT_CF9_SAFE_STR      "cf9_safe"

Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference.

The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both.

Regards,
-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-12  3:50 ` [PATCH] reboot: Fix variable assignments in type_store Nathan Chancellor
  2020-11-12 11:26   ` Matteo Croce
  2020-11-12 23:13   ` Andrew Morton
@ 2020-11-13 15:56   ` Petr Mladek
  2 siblings, 0 replies; 19+ messages in thread
From: Petr Mladek @ 2020-11-13 15:56 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: mcroce, akpm, arnd, keescook, linux-kernel, linux,
	pasha.tatashin, rppt, tyhicks, ndesaulniers, clang-built-linux

On Wed 2020-11-11 20:50:23, Nathan Chancellor wrote:
> Clang warns:
> 
> kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_TRIPLE;
>                             ~ ^~~~~~~~~~~
> kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_KBD;
>                             ~ ^~~~~~~~
> kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_BIOS;
>                             ~ ^~~~~~~~~
> kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_ACPI;
>                             ~ ^~~~~~~~~
> kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_EFI;
>                             ~ ^~~~~~~~
> kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_CF9_FORCE;
>                             ~ ^~~~~~~~~~~~~~
> kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
>                 reboot_mode = BOOT_CF9_SAFE;
>                             ~ ^~~~~~~~~~~~~
> 7 warnings generated.
> 
> It seems that these assignment should be to reboot_type, not
> reboot_mode. Fix it so there are no more warnings and the code works
> properly.
> 
> Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  kernel/reboot.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index deba133a071b..8599d0d44aec 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		return -EPERM;
>  
>  	if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> -		reboot_mode = BOOT_TRIPLE;
> +		reboot_type = BOOT_TRIPLE;
>  	else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> -		reboot_mode = BOOT_KBD;
> +		reboot_type = BOOT_KBD;
>  	else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> -		reboot_mode = BOOT_BIOS;
> +		reboot_type = BOOT_BIOS;
>  	else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> -		reboot_mode = BOOT_ACPI;
> +		reboot_type = BOOT_ACPI;
>  	else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> -		reboot_mode = BOOT_EFI;
> +		reboot_type = BOOT_EFI;
>  	else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> -		reboot_mode = BOOT_CF9_FORCE;
> +		reboot_type = BOOT_CF9_FORCE;
>  	else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> -		reboot_mode = BOOT_CF9_SAFE;
> +		reboot_type = BOOT_CF9_SAFE;
>  	else
>  		return -EINVAL;

Great catch! I guess that it has been a cut&paste mistake when writing
the code.

I feel shame that I have missed it. I think that I have even tested it
but I probably tried only mode_store() and mode_read().

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13  2:58             ` Matteo Croce
@ 2020-11-13 20:06               ` Petr Mladek
  2020-11-13 21:28                 ` Matteo Croce
  0 siblings, 1 reply; 19+ messages in thread
From: Petr Mladek @ 2020-11-13 20:06 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Andrew Morton, Nathan Chancellor, Arnd Bergmann, Kees Cook,
	linux-kernel, Guenter Roeck, Pavel Tatashin, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri 2020-11-13 03:58:49, Matteo Croce wrote:
> On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:
> >
> > > At this point, since 'pci' enables BOOT_CF9_FORCE type and
> > > BOOT_CF9_SAFE is not user selectable, should I simply leave only
> > > 'pci'?
> > > 
> > > This way, we'll have the same set of options for both sysfs and kernel cmdline.
> >
> > Well, you're the reboot expert ;)
> >
> 
> So honored! :)
> 
> > But my $0.02 is yes, let's keep the command-line and sysfs interfaces
> > in sync and cover it all in documentation.  It would of course be
> > problematic to change the existing reboot= interface.
> >
> > I assume that means doing this?
> >
> > - #define BOOT_CF9_FORCE_STR     "cf9_force"
> > + #define BOOT_CF9_FORCE_STR     "pci"
> > - #define BOOT_CF9_SAFE_STR      "cf9_safe"
> 
> Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference.
> 
> The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both.

The question is whether we should modify/allow to set these values at
all.

Anyway, we must prevent them on non-x86 architectures because
the reboot behavior would be undefined there. They could probably
make a mess even on many x86-architectures.

I have to admit it has become much more complicated than I thought.
It brings back Andrew's original question whether this interface is
really needed. Are you going to use in the real life?

The interface might do more harm then good when it allows to set
reboot_type that is not normally accessible or disable it when
it is strictly needed.

Anyway, we should get input from some x86-experts about the BOOT_CF9
values.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13 20:06               ` Petr Mladek
@ 2020-11-13 21:28                 ` Matteo Croce
  2020-11-18 11:47                   ` Petr Mladek
  0 siblings, 1 reply; 19+ messages in thread
From: Matteo Croce @ 2020-11-13 21:28 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andrew Morton, Nathan Chancellor, Arnd Bergmann, Kees Cook,
	linux-kernel, Guenter Roeck, Pavel Tatashin, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri, Nov 13, 2020 at 9:06 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2020-11-13 03:58:49, Matteo Croce wrote:
> > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:
> > >
> > > > At this point, since 'pci' enables BOOT_CF9_FORCE type and
> > > > BOOT_CF9_SAFE is not user selectable, should I simply leave only
> > > > 'pci'?
> > > >
> > > > This way, we'll have the same set of options for both sysfs and kernel cmdline.
> > >
> > > Well, you're the reboot expert ;)
> > >
> >
> > So honored! :)
> >
> > > But my $0.02 is yes, let's keep the command-line and sysfs interfaces
> > > in sync and cover it all in documentation.  It would of course be
> > > problematic to change the existing reboot= interface.
> > >
> > > I assume that means doing this?
> > >
> > > - #define BOOT_CF9_FORCE_STR     "cf9_force"
> > > + #define BOOT_CF9_FORCE_STR     "pci"
> > > - #define BOOT_CF9_SAFE_STR      "cf9_safe"
> >
> > Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference.
> >
> > The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both.
>
> The question is whether we should modify/allow to set these values at
> all.
>
> Anyway, we must prevent them on non-x86 architectures because
> the reboot behavior would be undefined there. They could probably
> make a mess even on many x86-architectures.
>

That's right, but the same can be obtained by passing 'reboot=pci'  on
non x86 machines: the cmdline parsing is generic and will set
reboot_type on all arches.

> I have to admit it has become much more complicated than I thought.
> It brings back Andrew's original question whether this interface is
> really needed. Are you going to use in the real life?
>

Yes, there are some cases.
Not to mention complex use cases like let persist some memory regions,
or change the page size,
if a network driver fails to rmmod with the infamous
"unregistered_netdevice: waiting for wlan0 to become free",
enabling force on the fly allows to reboot the machine.


> The interface might do more harm then good when it allows to set
> reboot_type that is not normally accessible or disable it when
> it is strictly needed.
>

I looked at the reboot_type usage, there isn't any reference outside
arch/x86. In fact, the parameter is just ignored:

# uname -m
aarch64
# cat /proc/cmdline
console=ttyS0,115200n8 reboot=pci
# reboot -ff
Rebooting.
[   43.893833] reboot: Restarting system

The same applies for reboot_force, the only flags available on
different architectures are reboot_mode and reboot_cpu.
We could hide some handlers for some architectures. We save some
space, and avoid letting the user set flags which do nothing.

> Anyway, we should get input from some x86-experts about the BOOT_CF9
> values.
>

Sure, x86@kernel.org ?

Regards,
-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] reboot: Fix variable assignments in type_store
  2020-11-13 21:28                 ` Matteo Croce
@ 2020-11-18 11:47                   ` Petr Mladek
  0 siblings, 0 replies; 19+ messages in thread
From: Petr Mladek @ 2020-11-18 11:47 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Andrew Morton, Nathan Chancellor, Arnd Bergmann, Kees Cook,
	linux-kernel, Guenter Roeck, Pavel Tatashin, Mike Rapoport,
	Tyler Hicks, Nick Desaulniers, clang-built-linux

On Fri 2020-11-13 22:28:18, Matteo Croce wrote:
> On Fri, Nov 13, 2020 at 9:06 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Fri 2020-11-13 03:58:49, Matteo Croce wrote:
> > > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >
> > > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <mcroce@linux.microsoft.com> wrote:
> > > >
> > > > > At this point, since 'pci' enables BOOT_CF9_FORCE type and
> > > > > BOOT_CF9_SAFE is not user selectable, should I simply leave only
> > > > > 'pci'?
> > > > >
> > > > > This way, we'll have the same set of options for both sysfs and kernel cmdline.
> > > >
> > > > Well, you're the reboot expert ;)
> > > >
> > >
> > > So honored! :)
> > >
> > > > But my $0.02 is yes, let's keep the command-line and sysfs interfaces
> > > > in sync and cover it all in documentation.  It would of course be
> > > > problematic to change the existing reboot= interface.
> > > >
> > > > I assume that means doing this?
> > > >
> > > > - #define BOOT_CF9_FORCE_STR     "cf9_force"
> > > > + #define BOOT_CF9_FORCE_STR     "pci"
> > > > - #define BOOT_CF9_SAFE_STR      "cf9_safe"
> > >
> > > Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference.
> > >
> > > The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both.
> >
> > The question is whether we should modify/allow to set these values at
> > all.
> >
> > Anyway, we must prevent them on non-x86 architectures because
> > the reboot behavior would be undefined there. They could probably
> > make a mess even on many x86-architectures.
> >
> 
> That's right, but the same can be obtained by passing 'reboot=pci'  on
> non x86 machines: the cmdline parsing is generic and will set
> reboot_type on all arches.

Fair enough. Ah, I mixed reboot_type and reboot_mode and looked
into wrong part of reboot_setup().

> I looked at the reboot_type usage, there isn't any reference outside
> arch/x86. In fact, the parameter is just ignored:
> 
> # uname -m
> aarch64
> # cat /proc/cmdline
> console=ttyS0,115200n8 reboot=pci
> # reboot -ff
> Rebooting.
> [   43.893833] reboot: Restarting system

Good to know. Thanks for checking.

> The same applies for reboot_force, the only flags available on
> different architectures are reboot_mode and reboot_cpu.
> We could hide some handlers for some architectures. We save some
> space, and avoid letting the user set flags which do nothing.

I am fine with the current patchset after all. We could always make
it more safe when people hit it in the real life. All these
worries were because I thought that this interface allowed
to set values that were not possible before.


> > Anyway, we should get input from some x86-experts about the BOOT_CF9
> > values.
> 
> Sure, x86@kernel.org ?

Yes but I do not resist on it any longer. Just if you were going to send
another version just by chance then it would be nice to CC x86.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4] reboot: allow to specify reboot mode via sysfs
  2020-11-10 20:27 [PATCH v4] reboot: allow to specify reboot mode via sysfs Matteo Croce
  2020-11-12  3:50 ` [PATCH] reboot: Fix variable assignments in type_store Nathan Chancellor
@ 2021-02-22 10:03 ` Pavel Machek
  2021-02-22 13:12   ` Matteo Croce
  1 sibling, 1 reply; 19+ messages in thread
From: Pavel Machek @ 2021-02-22 10:03 UTC (permalink / raw)
  To: Matteo Croce
  Cc: linux-kernel, Mike Rapoport, Guenter Roeck, Arnd Bergmann,
	Petr Mladek, Pavel Tatashin, Kees Cook, Andrew Morton,
	Tyler Hicks

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

On Tue 2020-11-10 21:27:46, 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.

We already have a reboot syscall, do we need sysfs interface, too?


> +What:		/sys/kernel/reboot/mode
> +Date:		November 2020
> +KernelVersion:	5.11
> +Contact:	Matteo Croce <mcroce@microsoft.com>
> +Description:	Reboot mode. Valid values are: cold warm hard soft gpio


> +What:		/sys/kernel/reboot/type
> +Date:		November 2020
> +KernelVersion:	5.11
> +Contact:	Matteo Croce <mcroce@microsoft.com>
> +Description:	Reboot type. Valid values are: bios acpi kbd triple efi pci

what is difference between mode and type? What is difference between
cold and hard for example? WTF is gpio?

> +What:		/sys/kernel/reboot/cpu
> +Date:		November 2020
> +KernelVersion:	5.11
> +Contact:	Matteo Croce <mcroce@microsoft.com>
> +Description:	CPU number to use to reboot.

Why should user care about this?

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4] reboot: allow to specify reboot mode via sysfs
  2021-02-22 10:03 ` [PATCH v4] reboot: allow to specify reboot mode via sysfs Pavel Machek
@ 2021-02-22 13:12   ` Matteo Croce
  0 siblings, 0 replies; 19+ messages in thread
From: Matteo Croce @ 2021-02-22 13:12 UTC (permalink / raw)
  To: Pavel Machek
  Cc: linux-kernel, Mike Rapoport, Guenter Roeck, Arnd Bergmann,
	Petr Mladek, Pavel Tatashin, Kees Cook, Andrew Morton,
	Tyler Hicks

On Mon, Feb 22, 2021 at 11:03 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Tue 2020-11-10 21:27:46, 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.
>
> We already have a reboot syscall, do we need sysfs interface, too?
>
>
> > +What:                /sys/kernel/reboot/mode
> > +Date:                November 2020
> > +KernelVersion:       5.11
> > +Contact:     Matteo Croce <mcroce@microsoft.com>
> > +Description: Reboot mode. Valid values are: cold warm hard soft gpio
>
>
> > +What:                /sys/kernel/reboot/type
> > +Date:                November 2020
> > +KernelVersion:       5.11
> > +Contact:     Matteo Croce <mcroce@microsoft.com>
> > +Description: Reboot type. Valid values are: bios acpi kbd triple efi pci
>
> what is difference between mode and type? What is difference between
> cold and hard for example? WTF is gpio?
>
> > +What:                /sys/kernel/reboot/cpu
> > +Date:                November 2020
> > +KernelVersion:       5.11
> > +Contact:     Matteo Croce <mcroce@microsoft.com>
> > +Description: CPU number to use to reboot.
>
> Why should user care about this?
>

Mode is the reboot mode (soft, warm, cold, gpio), and type is an x86
specific type, (bios, acpi, uefi, etc.).
I never used GPIO reboot but it's used by some ARM devices.

I didn't invent anything from scratch, I just transposed the settings
available from the kernel command line (see
Documentation/admin-guide/kernel-parameters.txt) to sysfs.
Everithing was already tunable before, like the CPU used during reboot.

-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-02-22 14:07 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-10 20:27 [PATCH v4] reboot: allow to specify reboot mode via sysfs Matteo Croce
2020-11-12  3:50 ` [PATCH] reboot: Fix variable assignments in type_store Nathan Chancellor
2020-11-12 11:26   ` Matteo Croce
2020-11-12 17:49     ` Nathan Chancellor
2020-11-12 17:59       ` Matteo Croce
2020-11-12 18:09         ` Nathan Chancellor
2020-11-12 23:13   ` Andrew Morton
2020-11-13  0:20     ` Matteo Croce
2020-11-13  0:41       ` Matteo Croce
2020-11-13  1:18       ` Andrew Morton
2020-11-13  1:38         ` Matteo Croce
2020-11-13  2:46           ` Andrew Morton
2020-11-13  2:58             ` Matteo Croce
2020-11-13 20:06               ` Petr Mladek
2020-11-13 21:28                 ` Matteo Croce
2020-11-18 11:47                   ` Petr Mladek
2020-11-13 15:56   ` Petr Mladek
2021-02-22 10:03 ` [PATCH v4] reboot: allow to specify reboot mode via sysfs Pavel Machek
2021-02-22 13:12   ` Matteo Croce

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).