On Thu, Oct 21, 2021 at 07:58:46PM PDT, Rob Herring wrote:
On Thu, Oct 21, 2021 at 9:00 PM Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
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.
I skimmed this, and overall I like the approach.
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.
One shortcoming of this series is that it doesn't automatically apply
universally across all busses and drivers -- patch 5 enables support
for platform devices, but similar changes would be required for
support in other busses (e.g. in of_register_spi_devices(),
of_i2c_register_devices(), etc.) and drivers that instantiate DT
devices. Since at present a "reserved" status is treated as
equivalent to "disabled" and this series preserves that status quo in
those cases I'd hope this wouldn't be considered a deal-breaker, but
a thing to be aware of at least.
Greg: I know on [1] you had commented nack-ing the addition of boolean
function parameters; patch 4 adds a flags mask instead in an analogous
situation. I'm not certain how much of an improvement you'd consider
that (hopefully at least slightly better, in that the arguments passed
at the call site are more self-explanatory); if that's still
unsatisfactory I'd welcome any suggested alternatives.
Can't we add a flag bit in struct device to reflect manual binding?
bind will set it and unbind clears it.
I considered this (and actually drafted up a version that did exactly
that), but it seemed like turning a parameter-passing problem into a
state-maintenance problem (finding all the places that flag would need
to be cleared and ensuring newly-added ones don't get missed, which
unlike a function parameter the compiler can't really check for us).
Given that the live range (definition to use) of that value is entirely
within the codepath of a single call-chain (bind_store() ->
device_driver_attach() -> __driver_probe_device()), continuing to
maintain that state beyond that call chain seemed like unnecessary
complexity to me, but if there's a consensus that that would actually be
preferable I can certainly do it that way instead.
Zev
