linux-kernel - Re: [RFC][PATCH 0/3] SRAM reboot mode driver

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: <56A884B9.5020201@mentor.com>
Date:	Wed, 27 Jan 2016 10:50:01 +0200
From:	Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
To:	John Stultz <john.stultz@...aro.org>,
	lkml <linux-kernel@...r.kernel.org>
CC:	Andy Yan <andy.yan@...k-chips.com>, Rob Herring <robh@...nel.org>,
	Arnd Bergmann <arnd@...db.de>,
	Thierry Reding <treding@...dia.com>,
	Heiko Stübner <heiko@...ech.de>,
	Caesar Wang <wxt@...k-chips.com>,
	Kees Cook <keescook@...omium.org>,
	Guodong Xu <guodong.xu@...aro.org>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Vishal Bhoj <vishal.bhoj@...aro.org>,
	Bjorn Andersson <bjorn.andersson@...aro.org>,
	<devicetree@...r.kernel.org>,
	Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC][PATCH 0/3] SRAM reboot mode driver

Hi John,

On 27.01.2016 02:37, John Stultz wrote:
> This patchset extends on Andy Yan's reboot mode driver
> work from here: https://lkml.org/lkml/2016/1/12/315
> 
> It adds reboot mode/reason support for devices that use
> an SRAM location to communicate with the bootloader.
> 
> Doing this via an SRAM subnode was a suggestion from
> Arnd, but I worry this implementation isn't yet ideal,
> since I spent quite a bit of time futzing with getting
> the sram dts entry to work properly. So I suspect there
> will be a number of suggestions for improvements.

sorry, I missed that discussion, what are the problems you've
encountered?
Probably SRAM driver is ready to serve your purposes?

I believe I'm screwing up some HiSilicon specifics, but here
is a rough draft, which you may find helpful:

1. Add SRAM node with defined reboot-reason area:

---8<---
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index 8185251..c35f5ed 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -31,6 +31,20 @@
 		device_type = "memory";
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+	sram@...1000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x05f01000 0x0 0x00001000>;
+		ranges = <0x0 0x0 0x05f01000 0x00001000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		reboot-reason: reboot-reason {
+			reg = <0x0 0x4>;
+			pool;
+		};
+	};
 };

 &uart2 {
---8<---

2. Reference reboot-reason node from reboot mode driver's node,
probably you need to add an optional property (here "reason-storage") to its
binding:

---8<---
		reboot-mode {
			compatible = "syscon-reboot-mode";
[snip]
			reason-storage = <&reboot-reason>;
		}
---8<---

3. In reboot-mode driver read or write to that SRAM partition:

---8<---
	storage = of_gen_pool_get(reboot_mode_np, "reason-storage", 0);
	if (!storage)
		goto no_storage;

	size = gen_pool_size(storage);
        base = gen_pool_alloc(storage, pool_size);
	if (!base)
		return -EINVAL;

	writel(0x77665501, base);
	gen_pool_free(storage, base, size);

no_storage:
	return 0;
---8<---

There is one noticeable benefit of this approach - you don't need
to write another driver and you don't need to add another DT binding.

With best wishes,
Vladimir

> Again, this series dependson Andy's patch set above,
> but also was developed & tested against the 4.1 based
> hikey tree, so at least the hikey dts patch won't apply.
> I'm mostly sending this out for just a rough initial
> review of the dts and conceptual usage of sram subnodes.
> 
> Also, it was pointed out that the hikey dts entry for
> this really should be added by the UEFI firmware, since
> alternative bootloaders may be used which do not support
> this feature. So the hikey dts patch isn't likely to ever
> go upstream, but its a useful illustration for how other
> devices might use this driver.
> 
> Cc: Andy Yan <andy.yan@...k-chips.com>
> Cc: Rob Herring <robh@...nel.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Thierry Reding <treding@...dia.com>
> Cc: Heiko Stübner <heiko@...ech.de>
> Cc: Caesar Wang <wxt@...k-chips.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Guodong Xu <guodong.xu@...aro.org>
> Cc: Haojian Zhuang <haojian.zhuang@...aro.org>
> Cc: Vishal Bhoj <vishal.bhoj@...aro.org>
> Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
> Cc: devicetree@...r.kernel.org
> Cc: Android Kernel Team <kernel-team@...roid.com>
> 
> John Stultz (3):
>   dt-bindings: power: reset: Add document for sram-reboot-mode driver
>   power: reset: Add sram-reboot-mode driver
>   dts: hikey: Add hikey support for sram-reboot-mode
> 
>  .../bindings/power/reset/sram-reboot-mode.txt      | 47 +++++++++++++++
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     | 36 ++++++++++++
>  arch/arm64/configs/hikey_defconfig                 |  3 +
>  drivers/power/reset/Kconfig                        |  9 +++
>  drivers/power/reset/Makefile                       |  1 +
>  drivers/power/reset/sram-reboot-mode.c             | 66 ++++++++++++++++++++++
>  6 files changed, 162 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/sram-reboot-mode.txt
>  create mode 100644 drivers/power/reset/sram-reboot-mode.c
>