| Message ID | 20211022020032.26980-2-zev@bewilderbeest.net |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | driver core, of: support for reserved devices | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 103 lines checked |
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
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
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
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)) 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); + + return res; +} +EXPORT_SYMBOL(of_device_is_reserved); + /** * of_device_is_big_endian - check if a device has BE registers * diff --git a/include/linux/of.h b/include/linux/of.h index 6f1c41f109bb..aa9762da5e7c 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -345,6 +345,7 @@ extern int of_device_is_compatible(const struct device_node *device, extern int of_device_compatible_match(struct device_node *device, const char *const *compat); extern bool of_device_is_available(const struct device_node *device); +extern bool of_device_is_reserved(const struct device_node *device); extern bool of_device_is_big_endian(const struct device_node *device); extern const void *of_get_property(const struct device_node *node, const char *name, @@ -707,6 +708,11 @@ static inline bool of_device_is_available(const struct device_node *device) return false; } +static inline bool of_device_is_reserved(const struct device_node *device) +{ + return false; +} + static inline bool of_device_is_big_endian(const struct device_node *device) { return false;
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(-)