linux-kernel - Re: [RFC][PATCH] misc: Introduce reboot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1858873.xsM5XHIlTX@wuerfel>
Date:	Tue, 08 Dec 2015 22:52:08 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	John Stultz <john.stultz@...aro.org>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Vinay Simha BN <simhavcs@...il.com>,
	Bjorn Andersson <bjorn.andersson@...ymobile.com>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	devicetree@...r.kernel.org,
	Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC][PATCH] misc: Introduce reboot_reason driver

On Tuesday 08 December 2015 13:29:22 John Stultz wrote:

> diff --git a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> index 5183d18..ee5dcb7 100644
> --- a/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> +++ b/arch/arm/boot/dts/qcom-apq8064-nexus7-flo.dts
> @@ -282,6 +282,15 @@
>  			};
>  		};
>  
> +		reboot_reason: reboot_reason@...3f65c {
> +			compatible		= "reboot_reason";

This is not a good compatible string. There should generally be a vendor
name associated with it (use "linux," if nothing else, and you should have
'-' instead of '_'.

> +			reg			= <0x2A03F65C 0x4>;

This may easily conflict with the device it is part of. We should have
non-overlapping register areas in general. For the example you are
looking at, which register block is this?

> +
> +/* Types of reasons */
> +static enum {
> +	NONE,
> +	BOOTLOADER,
> +	RECOVERY,
> +	OEM,
> +	MAX_REASONS
> +} __maybe_unused reason_types;

The variable seems to always be unused, not just "__maybe_unused". Maybe remove it?

> +static int reboot_reason(struct notifier_block *nb, unsigned long action,
> +								void *data)
> +{
> +	char *cmd = (char *)data;
> +	long reason = reasons[NONE];
> +
> +	if (!reboot_reason_addr)
> +		return NOTIFY_DONE;
> +
> +	if (cmd != NULL) {
> +		if (!strncmp(cmd, "bootloader", 10))
> +			reason = reasons[BOOTLOADER];
> +		else if (!strncmp(cmd, "recovery", 8))
> +			reason = reasons[RECOVERY];
> +		else if (!strncmp(cmd, "oem-", 4)) {
> +			unsigned long code;
> +
> +			if (!kstrtoul(cmd+4, 0, &code))
> +				reason = reasons[OEM] | (code & 0xff);
> +		}
> +	}
> +
> +	if (reason != -1)
> +		writel(reason, reboot_reason_addr);
> +	return NOTIFY_DONE;
> +}

Will this reboot the machine?

> +	/* Install the notifier */
> +	restart_nb.notifier_call = reboot_reason;
> +	restart_nb.priority = 256;
> +	if (register_restart_handler(&restart_nb)) {

If not, you should use register_reboot_notifier() rather than
register_restart_handler(). The former gets called to do something
just before rebooting, while the latter attempts to actually reboot
the machine.

> +static int __init reboot_reason_init(void)
> +{
> +	return platform_driver_register(&reboot_reason_driver);
> +}
> +arch_initcall(reboot_reason_init);

Why this early? If it can be a normal device_initcall, you can use
module_platform_driver().

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/