Re: [PATCH 1/5] of: base: add function to check for status = "reserved"

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@bewilderbeest.net>
> > > ---
> > >  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@mail.gmail.com/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