On 10/25/21 12:53 AM, Frank Rowand wrote:

On 10/22/21 4:00 AM, Zev Weiss wrote:

quoted

On Thu, Oct 21, 2021 at 11:50:07PM PDT, Greg Kroah-Hartman wrote:

quoted

On Thu, Oct 21, 2021 at 07:00:27PM -0700, Zev Weiss wrote:

quoted

Hello all,

This series is another incarnation of a couple other patchsets I've
posted recently [0, 1], but again different enough in overall
structure that I'm not sure it's exactly a v2 (or v3).

As compared to [1], it abandons the writable binary sysfs files and at
Frank's suggestion returns to an approach more akin to [0], though
without any driver-specific (aspeed-smc) changes, which I figure might
as well be done later in a separate series once appropriate
infrastructure is in place.

The basic idea is to implement support for a status property value
that's documented in the DT spec [2], but thus far not used at all in
the kernel (or anywhere else I'm aware of): "reserved".  According to
the spec (section 2.3.4, Table 2.4), this status:

  Indicates that the device is operational, but should not be used.
  Typically this is used for devices that are controlled by another
  software component, such as platform firmware.

With these changes, devices marked as reserved are (at least in some
cases, more on this later) instantiated, but will not have drivers
bound to them unless and until userspace explicitly requests it by
writing the device's name to the driver's sysfs 'bind' file.  This
enables appropriate handling of hardware arrangements that can arise
in contexts like OpenBMC, where a device may be shared with another
external controller not under the kernel's control (for example, the
flash chip storing the host CPU's firmware, shared by the BMC and the
host CPU and exclusively under the control of the latter by default).
Such a device can be marked as reserved so that the kernel refrains
from touching it until appropriate preparatory steps have been taken
(e.g. BMC userspace coordinating with the host CPU to arbitrate which
processor has control of the firmware flash).

Patches 1-3 provide some basic plumbing for checking the "reserved"
status of a device, patch 4 is the main driver-core change, and patch
5 tweaks the OF platform code to not skip reserved devices so that
they can actually be instantiated.

Again, the driver core should not care about this, that is up to the bus
that wants to read these "reserved" values and do something with them or
not (remember the bus is the thing that does the binding, not the driver
core).

But are you sure you are using the "reserved" field properly?

Well, thus far both Rob Herring and Oliver O'Halloran (originator of the "reserved" status in the DT spec, whom I probably should have CCed earlier, sorry) have seemed receptive to this interpretation of it, which I'd hope would lend it some credence.

I am not on board with this interpretation.  To me, if the value of
status is "reserved", then the Linux kernel should _never_ use the
device described by the node.

If a "reserved" node is usable by the Linux kernel, then the specification
should be updated to allow this.  And the specification should probably
be expanded to either discuss how to describe the coordination between
multiple entities or state that the coordination is outside of the
specification and will be implemention dependent.

Maybe a value of "reserved-sharable" should be added to the standard.
This would indicate that the node is operational and controlled by
another software component, but is available to the operating system
after requesting permission or being granted permission from the other
software component.

The exact method of requesting permission or being granted permission
could either be driver specific, or the driver binding could
include one or more additional properties to describe the method.

One thing that I am wary of is the possibility of a proliferation of
status checks changing from "okay" to "okay" || ("reserved" and the
state of the driver is that permission has been granted).

From a simplicity of coding view, it is really tempting to dynamically
change the value of the status property from "reserved-sharable"
to "okay" when the other software component grants permission to
use the device, so that status checks will magically allow use
instead of blocking use.  I do not like changing the value of the
status property dynamically because the devicetree is supposed to
describe hardware (and communicate information from the firmware
to the operating system), not to actively maintain state.

-Frank

I am wary of the complexity of the operating system treating a node as
reserved at initial boot, then at some point via coordination with
some other entity starting to use the node.  It is not too complex if
the node is a leaf node with no links (phandles) to or from any other node,
but as soon as links to or from other nodes exist, then other drivers or
subsystems may need to be aware of when the node is available to the
operating system or given back to the other entity then any part of the
operating system has to coordinate in that state transition.  This is
driving a lot of my caution that we be careful to create architecture
and not an ad hoc hack.

-Frank

quoted

quoted

You are
wanting to do "something" to the device to later on be able to then have
the kernel touch the device, while it seems that the reason for this
field is for the kernel to NEVER touch the device at all.  What will
break if you change this logic?

Given that there's no existing usage of or support for this status value anywhere I can see in the kernel, and that Oliver has indicated that it should be compatible with usage in OpenPower platform firmware, my expectation would certainly be that nothing would break, but if there are examples of things that could I'd be interested to see them.


Thanks,
Zev