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

[Petr Mladek <pmladek@suse.com>]

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

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

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

Please, split it.


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

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

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

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

and use here:

	case REBOOT_COLD:
		val = REBOOT_COLD_STR;

and then at the end

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


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

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


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

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


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



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

Same here

		var = BOOT_TRIPLE_STR;

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

and here:

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


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

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

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

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

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

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

Best Regards,
Petr