From: Matteo Croce <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+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 <[email protected]>
+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 <[email protected]>
+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 <[email protected]>
+Description: CPU number to use to reboot.
+
+What: /sys/kernel/reboot/force
+Date: November 2020
+KernelVersion: 5.11
+Contact: Matteo Croce <[email protected]>
+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
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 <[email protected]>
---
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
On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
<[email protected]> 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 <[email protected]>
> ---
> 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
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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Thu, Nov 12, 2020 at 6:49 PM Nathan Chancellor
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Thu, Nov 12, 2020 at 06:59:36PM +0100, Matteo Croce wrote:
> On Thu, Nov 12, 2020 at 6:49 PM Nathan Chancellor
> <[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor <[email protected]> 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?
On Fri, Nov 13, 2020 at 12:13 AM Andrew Morton
<[email protected]> wrote:
>
> On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor <[email protected]> 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
On Fri, Nov 13, 2020 at 1:20 AM Matteo Croce <[email protected]> wrote:
>
> On Fri, Nov 13, 2020 at 12:13 AM Andrew Morton
> <[email protected]> wrote:
> >
> > On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor <[email protected]> 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 <[email protected]>
--
per aspera ad upstream
On Fri, 13 Nov 2020 01:20:29 +0100 Matteo Croce <[email protected]> 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.
On Fri, Nov 13, 2020 at 2:18 AM Andrew Morton <[email protected]> wrote:
>
> On Fri, 13 Nov 2020 01:20:29 +0100 Matteo Croce <[email protected]> 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
On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <[email protected]> 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"
On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <[email protected]> wrote:
>
> On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <[email protected]> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
Best Regards,
Petr
On Fri 2020-11-13 03:58:49, Matteo Croce wrote:
> On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <[email protected]> wrote:
> >
> > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <[email protected]> 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
On Fri, Nov 13, 2020 at 9:06 PM Petr Mladek <[email protected]> wrote:
>
> On Fri 2020-11-13 03:58:49, Matteo Croce wrote:
> > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <[email protected]> wrote:
> > >
> > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <[email protected]> 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, [email protected] ?
Regards,
--
per aspera ad upstream
On Fri 2020-11-13 22:28:18, Matteo Croce wrote:
> On Fri, Nov 13, 2020 at 9:06 PM Petr Mladek <[email protected]> wrote:
> >
> > On Fri 2020-11-13 03:58:49, Matteo Croce wrote:
> > > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton <[email protected]> wrote:
> > > >
> > > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce <[email protected]> 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, [email protected] ?
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
On Tue 2020-11-10 21:27:46, Matteo Croce wrote:
> From: Matteo Croce <[email protected]>
>
> 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 <[email protected]>
> +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 <[email protected]>
> +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 <[email protected]>
> +Description: CPU number to use to reboot.
Why should user care about this?
--
http://www.livejournal.com/~pavelmachek
On Mon, Feb 22, 2021 at 11:03 AM Pavel Machek <[email protected]> wrote:
>
> On Tue 2020-11-10 21:27:46, Matteo Croce wrote:
> > From: Matteo Croce <[email protected]>
> >
> > 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 <[email protected]>
> > +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 <[email protected]>
> > +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 <[email protected]>
> > +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