Re: [PATCH 0/5] driver core, of: support for reserved devices

[Zev Weiss]

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