linux-kernel - Re: [PATCH] of: Add generic handling for hardware incomplete fail state

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: <20160412220244.GK5995@atomide.com>
Date:	Tue, 12 Apr 2016 15:02:45 -0700
From:	Tony Lindgren <tony@...mide.com>
To:	Frank Rowand <frowand.list@...il.com>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
	Nishanth Menon <nm@...com>, Tero Kristo <t-kristo@...com>,
	Tom Rini <trini@...sulko.com>
Subject: Re: [PATCH] of: Add generic handling for hardware incomplete fail
 state

* Frank Rowand <frowand.list@...il.com> [160412 14:42]:
> On 4/12/2016 1:34 PM, Tony Lindgren wrote:
> > 
> > OK thanks for the clarification. I don't see why "fail-hw-incomplete"
> > could not be set dynamically during the probe in some cases based
> > on the SoC revision detection for example. So from that point of
> > view using status with the "fail-sss" logic would make more sense.
> 
> If the probe detects that the device should only be power managed
> based on the SoC revision, then it would simply be one more
> test added at the top of probe.  The patch would change from:
> 
>    if (of_device_is_incomplete(pdev->dev.of_node)) {
> 
> to:
> 
>    if (of_device_is_incomplete(pdev->dev.of_node) || socrev == XXX) {
> 
> That code would be the same whether the property involved was
> status or something else.

Yeah that should work if we had a generic way to get the runtime
socrev somehow :) I guess the closest thing is the ARM system_rev.

> >> I would prefer to come up with a new boolean property (with a
> >> standard name that any node binding could choose to implement)
> >> that says something like "only power management is available for
> >> this node, do not attempt to use any other feature of the node".
> > 
> > Heh that's going to be a long property name :) How about
> > unusable-incomplete-idle-only :)
> 
> Or even pm-only.  Maybe I got a little carried away with my
> verbosity. :)

That works for me unless somebody comes up with a better one.
I can only think unusable-for-io, which is no better.

> >> With that change, the bulk of your patch looks good, with
> >> minor changes:
> >>
> >>   __of_device_is_available() would not need to change.
> >>
> >>   __of_device_is_incomplete() would change to check the new
> >>   boolean property.  (And I would suggest renaming it to
> >>   something that conveys it is ok to power manage the
> >>   device, but do not do anything else to the device.)
> > 
> > I'm fine with property too, but the runtime probe fail state
> > changes worry me a bit with that one.
> 
> I don't understand what the concern is.  The change I suggested
> would use exactly the same code for probe as the example patch
> you provided, but just with a slight name change for the function.

I guess I'm just wondering if using property vs status will make
things harder to change during runtime. For example, let's assume
u-boot needs to set some devices to "pm-only" state based on the
SoC revision on a board.

> > I think Rob also preferred to use the status though while we
> > chatted at ELC?
> 
> That is the impression I got too.  We'll have to see if I can
> convince him otherwise.

Yeah let's wait for his comments.

Tony