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

Zev Weiss
2021-10-22T07:38:47Z

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

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

Thanks,
Zev