linux-kernel - Re: [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey

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: <CALAqxLW=tmpmo7pfvqWijL60VQ8nCj47qW6cH5Hbgs0nwXKTnQ@mail.gmail.com>
Date:	Fri, 5 Aug 2016 15:51:20 -0700
From:	John Stultz <john.stultz@...aro.org>
To:	Rob Herring <robh@...nel.org>
Cc:	Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>,
	lkml <linux-kernel@...r.kernel.org>,
	Andy Yan <andy.yan@...k-chips.com>,
	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" <devicetree@...r.kernel.org>,
	Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey

On Fri, Aug 5, 2016 at 3:37 PM, Rob Herring <robh@...nel.org> wrote:
> On Fri, Aug 5, 2016 at 7:46 AM, Vladimir Zapolskiy
> <vladimir_zapolskiy@...tor.com> wrote:
>> Hi John,
>>
>> On 08/04/2016 02:05 AM, John Stultz wrote:
>>>
>>> Now that Andy's reboot reason core driver has landed, I wanted
>>> to resubmit a reworked version of my SRAM based reboot reason
>>> driver.
>>>
>>> This allows the kernel to communicate to the bootloader what mode
>>> it should reboot to using some reserved memory.
>>>
>>> Feedback would be very much appreciated!
>>
>>
>> in my opinion the taken approach is wrong, and I've already explained
>> why and how to rework your driver to shrink the change, please see
>> https://lkml.org/lkml/2016/1/27/133
>>
>> In this case I think that a SRAM device node should just contain
>> a plain description of partitions, compatible = "sram-reboot-mode" is
>> clearly not a device on "SRAM bus", it is not a device at all, so
>> please let's separate policy from mechanism
>
> Having a 2nd node for the driver is still not a device on a bus. It
> adds unneeded complexity to the binding IMO.
>
> The current approach also follows the model ramoops is using. Right
> now it's using reserved-memory, but that could easily be extended to
> SRAM region as well.
>
>> Because my proposed alternative approach separates policy from
>> mechanism, it for instanse allows to avoid overlappings on SRAM areas,
>> and still other drivers may serve as consumers of partitions on SRAM.
>
> You could still have multiple consumers and having a compatible string
> doesn't necessarily imply a driver. Though multiple consumers without
> something arbitrating access sounds like broken design to me.

So after running into some issues implementing the feedback that Bjorn
suggested, I realized we were going to need to not only extend the
sram driver to probe children, but we'd also have to make it a mfd so
it wouldn't reserve the entire range and the reboot reason driver
could map the memory.

That on top of the fact that we're already duplicating much of the
syscon-reboot-mode driver to work on sram, I decided to just start
over and use the syscon driver, which works fine here. All that is
needed is just adding it to the dts.

I know that its not exactly correct usage of the syscon driver, but it
starts to feel crazy almost completely duplicating the syscon driver
just to have it named sram.

thanks
-john