On Fri, Oct 22, 2021 at 12:38:40AM -0700, Zev Weiss wrote: > On Thu, Oct 21, 2021 at 11:43:23PM PDT, Greg Kroah-Hartman wrote: > > On Thu, Oct 21, 2021 at 07:00:28PM -0700, Zev Weiss wrote: > > > Per v0.3 of the Devicetree Specification [0]: > > > > > > 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. > > > > > > One use-case for this is in OpenBMC, where certain devices (such as a > > > BIOS flash chip) may be shared by the host and the BMC, but cannot be > > > accessed by the BMC during its usual boot-time device probing, because > > > they require additional (potentially elaborate) coordination with the > > > host to arbitrate which processor is controlling the device. > > > > > > Devices marked with this status should thus be instantiated, but not > > > have a driver bound to them or be otherwise touched. > > > > > > [0] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf > > > > > > Signed-off-by: Zev Weiss <zev@xxxxxxxxxxxxxxxxx> > > > --- > > > drivers/of/base.c | 56 +++++++++++++++++++++++++++++++++++++++------- > > > include/linux/of.h | 6 +++++ > > > 2 files changed, 54 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c > > > index 0ac17256258d..3bd7c5b8a2cc 100644 > > > --- a/drivers/of/base.c > > > +++ b/drivers/of/base.c > > > @@ -580,14 +580,16 @@ int of_machine_is_compatible(const char *compat) > > > EXPORT_SYMBOL(of_machine_is_compatible); > > > > > > /** > > > - * __of_device_is_available - check if a device is available for use > > > + * __of_device_check_status - check if a device's status matches a particular string > > > * > > > - * @device: Node to check for availability, with locks already held > > > + * @device: Node to check status of, with locks already held > > > + * @val: Status string to check for, or NULL for "okay"/"ok" > > > * > > > - * Return: True if the status property is absent or set to "okay" or "ok", > > > - * false otherwise > > > + * Return: True if status property exists and matches @val, or either "okay" > > > + * or "ok" if @val is NULL, or if status property is absent and @val is > > > + * "okay", "ok", or NULL. False otherwise. > > > */ > > > -static bool __of_device_is_available(const struct device_node *device) > > > +static bool __of_device_check_status(const struct device_node *device, const char *val) > > > { > > > const char *status; > > > int statlen; > > > @@ -596,17 +598,35 @@ static bool __of_device_is_available(const struct device_node *device) > > > return false; > > > > > > status = __of_get_property(device, "status", &statlen); > > > - if (status == NULL) > > > - return true; > > > + if (!status) { > > > + /* a missing status property is treated as "okay" */ > > > + status = "okay"; > > > + statlen = strlen(status) + 1; /* property lengths include the NUL terminator */ > > > + } > > > > > > if (statlen > 0) { > > > - if (!strcmp(status, "okay") || !strcmp(status, "ok")) > > > + if (!val && (!strcmp(status, "okay") || !strcmp(status, "ok"))) > > > + return true; > > > + else if (val && !strcmp(status, val)) > > > > > > Ick, where is this string coming from? The kernel or userspace or a > > device tree? This feels very wrong, why is the kernel doing parsing > > like this of different options that all mean the same thing? > > > > Which string do you mean by "this string"? 'status' comes from a property > of the device tree node; 'val' will be one of a small set of string > constants passed by the caller. Accepting either spelling of "okay"/"ok" > has been in place since 2008 (commit 834d97d45220, "[POWERPC] Add > of_device_is_available function"); using NULL as a shorthand for those two > strings was a suggestion that came up in review feedback on a previous > incarnation of these patches (https://lore.kernel.org/openbmc/CAL_Jsq+rKGv39zHTxNx0A7=X4K48nXRLqWonecG5SobdJq3yKw@xxxxxxxxxxxxxx/T/#u). I was referring to "okay". And if this really is a "we take either" type of thing, shouldn't there be a single function to call for this type of test, much like we have some of the sysfs helpers? And what about using match_string() as well? > > > return true; > > > } > > > > > > return false; > > > } > > > > > > +/** > > > + * __of_device_is_available - check if a device is available for use > > > + * > > > + * @device: Node to check for availability, with locks already held > > > + * > > > + * Return: True if the status property is absent or set to "okay" or "ok", > > > + * false otherwise > > > + */ > > > +static bool __of_device_is_available(const struct device_node *device) > > > +{ > > > + return __of_device_check_status(device, NULL); > > > +} > > > + > > > /** > > > * of_device_is_available - check if a device is available for use > > > * > > > @@ -628,6 +648,26 @@ bool of_device_is_available(const struct device_node *device) > > > } > > > EXPORT_SYMBOL(of_device_is_available); > > > > > > +/** > > > + * of_device_is_reserved - check if a device is marked as reserved > > > + * > > > + * @device: Node to check for reservation > > > + * > > > + * Return: True if the status property is set to "reserved", false otherwise > > > + */ > > > +bool of_device_is_reserved(const struct device_node *device) > > > +{ > > > + unsigned long flags; > > > + bool res; > > > + > > > + raw_spin_lock_irqsave(&devtree_lock, flags); > > > + res = __of_device_check_status(device, "reserved"); > > > + raw_spin_unlock_irqrestore(&devtree_lock, flags); > > > > Why is this a "raw" spinlock? > > > > devtree_lock being a raw_spinlock_t appears to date from commit d6d3c4e65651 > ("OF: convert devtree lock from rw_lock to raw spinlock"); "required for > preempt-rt", according to Thomas Gleixner's commit message. > > > Where is this status coming from? > > > > This would be specified in a DT node, e.g. via something like: > > &somedev { > compatible = "foobar"; > status = "reserved"; > /* ... */ > }; > > > > + > > > + return res; > > > +} > > > +EXPORT_SYMBOL(of_device_is_reserved); > > > > EXPORT_SYMBOL_GPL()? > > > > Its closest existing sibling, of_device_is_available(), is a plain > EXPORT_SYMBOL(); if we want to convert things more broadly that'd be fine > with me, but having one be GPL-only and the other not seems like it'd be a > bit confusing and inconsistent? Ah, ok, you are following the rest of this file for this, and the locking stuff, sorry, I was not familiar with it. thanks, greg k-h
