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. Rob
