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

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

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?


>  			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?

Where is this status coming from?

> +
> +	return res;
> +}
> +EXPORT_SYMBOL(of_device_is_reserved);

EXPORT_SYMBOL_GPL()?

thanks,

greg k-h