Messages in this thread | | | Date | Thu, 21 Oct 2021 20:13:19 -0700 | From | Zev Weiss <> | Subject | Re: [PATCH 0/5] driver core, of: support for reserved devices |
| |
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@bewilderbeest.net> 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
| |