On Mon, Nov 9, 2020 at 3:16 PM Petr Mladek <pmla...@suse.com> wrote: > > On Fri 2020-11-06 21:07:04, Matteo Croce wrote: > > From: Matteo Croce <mcr...@microsoft.com> > > > > The kernel cmdline reboot= option offers some sort of control > > on how the reboot is issued. > > Add handles in sysfs to allow setting these reboot options, so they > > can be changed when the system is booted, other than at boot time. > > > > The handlers are under <sysfs>/kernel/reboot, can be read to > > get the current configuration and written to alter it. > > > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-kernel-reboot > > @@ -0,0 +1,26 @@ > > +What: /sys/kernel/reboot > > +Date: October 2020 > > +KernelVersion: 5.11 > > +Contact: Matteo Croce <mcr...@microsoft.com> > > +Description: Interface to set the kernel reboot mode, similarly to > > + what can be done via the reboot= cmdline option. > > + (see Documentation/admin-guide/kernel-parameters.txt) > > + > > +What: /sys/kernel/reboot/mode > > +What: /sys/kernel/reboot/type > > +What: /sys/kernel/reboot/cpu > > +What: /sys/kernel/reboot/force > > I do not see any file where it is accumulated this way. > It seems that each path is always described separately. >
I've found it in Documentation/ABI/testing/sysfs-kernel-mm-ksm > I am not sure if it is really needed. But it might be needed > when processing the API documentation. > > Please, split it. > Ok > > > + > > +Date: October 2020 > > +Contact: Matteo Croce <mcr...@microsoft.com> > > +Description: Tune reboot parameters. > > + > > + mode: Reboot mode. Valid values are: > > + cold warm hard soft gpio > > + > > + type: Reboot type. Valid values are: > > + bios acpi kbd triple efi pci > > + > > + cpu: CPU number to use to reboot. > > + > > + force: Force an immediate reboot. > > diff --git a/kernel/reboot.c b/kernel/reboot.c > > index e7b78d5ae1ab..b9e607517ae3 100644 > > --- a/kernel/reboot.c > > +++ b/kernel/reboot.c > > @@ -594,3 +594,196 @@ static int __init reboot_setup(char *str) > > return 1; > > } > > __setup("reboot=", reboot_setup); > > + > > +#ifdef CONFIG_SYSFS > > + > > +#define STARTS_WITH(s, sc) (!strncmp(s, sc, sizeof(sc)-1)) > > + > > +static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute > > *attr, char *buf) > > +{ > > + const char *val; > > + > > + switch (reboot_mode) { > > + case REBOOT_COLD: > > + val = "cold\n"; > > Using "\n" everywhere is weird. Also the same strings are > repeated in the next functions. > > I suggest to define them only once, e.g. > > #define REBOOT_COLD_STR "cold" > #define REBOOT_WARM_STR "warm" > > and use here: > > case REBOOT_COLD: > val = REBOOT_COLD_STR; > > and then at the end > > return sprintf(buf, "%s\n", val); > > Ok. > > + break; > > + case REBOOT_WARM: > > + val = "warm\n"; > > + break; > > + case REBOOT_HARD: > > + val = "hard\n"; > > + break; > > + case REBOOT_SOFT: > > + val = "soft\n"; > > + break; > > + case REBOOT_GPIO: > > + val = "gpio\n"; > > + break; > > + default: > > + val = "undefined\n"; > > + } > > + > > + return strscpy(buf, val, 10); > > "undefined\n" needs 11 bytes to store also the trailing '\0'. > Anyway, the buffer should be big enough for all variants. > > Oops, missed it. > > +} > > +static ssize_t mode_store(struct kobject *kobj, struct kobj_attribute > > *attr, > > + const char *buf, size_t count) > > +{ > > + if (!capable(CAP_SYS_BOOT)) > > + return -EPERM; > > + > > + if (STARTS_WITH(buf, "cold")) > > + reboot_mode = REBOOT_COLD; > > I would prefer to open code this and use strlen(). It will be obvious > what the code does immediately. And I am sure that compilators > will optimize out the strlen(). > > > if (strncmp(buf, REBOOT_COLD_STR, strlen(REBOOT_COLD_STR)) == 0) > reboot_mode = REBOOT_COLD; > else if (strncmp(buf, REBOOT_WARM_STR, strlen(REBOOT_WARM_STR) == 0) > reboot_mode = REBOOT_WARM; > ... > Yes, since they are constant. > > > > + else if (STARTS_WITH(buf, "warm")) > > + reboot_mode = REBOOT_WARM; > > + else if (STARTS_WITH(buf, "hard")) > > + reboot_mode = REBOOT_HARD; > > + else if (STARTS_WITH(buf, "soft")) > > + reboot_mode = REBOOT_SOFT; > > + else if (STARTS_WITH(buf, "gpio")) > > + reboot_mode = REBOOT_GPIO; > > + else > > + return -EINVAL; > > + > > + return count; > > +} > > +static struct kobj_attribute reboot_mode_attr = __ATTR_RW(mode); > > + > > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute > > *attr, char *buf) > > +{ > > + const char *val; > > + > > + switch (reboot_type) { > > + case BOOT_TRIPLE: > > + val = "triple\n"; > > Same here > > var = BOOT_TRIPLE_STR; > > > + break; > > + case BOOT_KBD: > > + val = "kbd\n"; > > + break; > > + case BOOT_BIOS: > > + val = "bios\n"; > > + break; > > + case BOOT_ACPI: > > + val = "acpi\n"; > > + break; > > + case BOOT_EFI: > > + val = "efi\n"; > > + break; > > + case BOOT_CF9_FORCE: > > + val = "cf9_force\n"; > > + break; > > + case BOOT_CF9_SAFE: > > + val = "cf9_safe\n"; > > + break; > > + default: > > + val = "undefined\n"; > > + } > > + > > + return strscpy(buf, val, 10); > > +} > > +static ssize_t type_store(struct kobject *kobj, struct kobj_attribute > > *attr, > > + const char *buf, size_t count) > > +{ > > + if (!capable(CAP_SYS_BOOT)) > > + return -EPERM; > > + > > + if (STARTS_WITH(buf, "triple")) > > + reboot_type = BOOT_TRIPLE; > > and here: > > if (strncmp(buf, REBOOT_TRIPLE_STR, strlen(REBOOT_TRIPLE_STR)) == 0) > reboot_type = REBOOT_TRIPLE; > > > > + else if (STARTS_WITH(buf, "kbd")) > > + reboot_type = BOOT_KBD; > > + else if (STARTS_WITH(buf, "bios")) > > + reboot_type = BOOT_BIOS; > > + else if (STARTS_WITH(buf, "acpi")) > > + reboot_type = BOOT_ACPI; > > + else if (STARTS_WITH(buf, "efi")) > > + reboot_type = BOOT_EFI; > > + else if (STARTS_WITH(buf, "cf9_force")) > > + reboot_type = BOOT_CF9_FORCE; > > + else if (STARTS_WITH(buf, "cf9_safe")) > > + reboot_type = BOOT_CF9_SAFE; > > + else > > + return -EINVAL; > > + > > + return count; > > +} > > +static struct kobj_attribute reboot_type_attr = __ATTR_RW(type); > > + > > +static ssize_t force_show(struct kobject *kobj, struct kobj_attribute > > *attr, char *buf) > > +{ > > + return sprintf(buf, "%d\n", reboot_force); > > +} > > +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute > > *attr, > > + const char *buf, size_t count) > > +{ > > + if (!capable(CAP_SYS_BOOT)) > > + return -EPERM; > > + > > + if (buf[0] != '0' && buf[0] != '1') > > + return -EINVAL; > > Please use kstrtobool() that supports also other boolean values, > for example, 'Y', 'n'. > Nice, will do. > > + rc = kstrtouint(buf, 0, &cpunum); > > + > > + reboot_force = buf[0] - '0'; > > + > > + return count; > > +} > > > +static int __init reboot_ksysfs_init(void) > > +{ > > + struct kobject *reboot_kobj; > > + int ret; > > + > > + reboot_kobj = kobject_create_and_add("reboot", kernel_kobj); > > + if (!reboot_kobj) > > + return -ENOMEM; > > + > > + ret = sysfs_create_group(reboot_kobj, &reboot_attr_group); > > + if (ret) { > > + kobject_put(reboot_kobj); > > + return ret; > > + } > > + > > + return 0; > > +} > > +core_initcall(reboot_ksysfs_init); > > There is no need to create the sysfs interface this early. In fact, it > might even break because the parent "kernel" node is defined > as core_initcall() as well. The order is not defined in this case. > > I would do it as sybsys_initcall() like or even late_initcall(). > I'll postpone it to late_initcall(). Thanks, -- per aspera ad upstream