* [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status @ 2021-11-08 8:48 Matthias Schiffer 2021-11-14 19:41 ` Frank Rowand 0 siblings, 1 reply; 6+ messages in thread From: Matthias Schiffer @ 2021-11-08 8:48 UTC (permalink / raw) To: Rob Herring, Frank Rowand; +Cc: devicetree, linux-kernel, Matthias Schiffer Allow fully disabling CPU nodes using status = "fail". Having no status property at all is still interpreted as "okay" as usual. This allows a bootloader to change the number of available CPUs (for example when a common DTS is used for SoC variants with different numbers of cores) without deleting the nodes altogether, which could require additional fixups to avoid dangling phandle references. References: - https://www.lkml.org/lkml/2020/8/26/1237 - https://www.spinics.net/lists/devicetree-spec/msg01007.html - https://github.com/devicetree-org/dt-schema/pull/61 Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/of/base.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/drivers/of/base.c b/drivers/of/base.c index 61de453b885c..4e9973627c8d 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device) } EXPORT_SYMBOL(of_device_is_available); +/** + * __of_device_is_disabled - check if a device has status "disabled" + * + * @device: Node to check status for, with locks already held + * + * Return: True if the status property is set to "disabled", + * false otherwise + * + * Most callers should use __of_device_is_available() instead, this function + * only exists due to the special interpretation of the "disabled" status for + * CPU nodes. + */ +static bool __of_device_is_disabled(const struct device_node *device) +{ + const char *status; + + if (!device) + return false; + + status = __of_get_property(device, "status", NULL); + if (status == NULL) + return false; + + return !strcmp(status, "disabled"); +} + /** * of_device_is_big_endian - check if a device has BE registers * @@ -817,6 +843,9 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) of_node_put(node); } for (; next; next = next->sibling) { + if (!__of_device_is_available(next) && + !__of_device_is_disabled(next)) + continue; if (!(of_node_name_eq(next, "cpu") || __of_node_is_type(next, "cpu"))) continue; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status 2021-11-08 8:48 [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status Matthias Schiffer @ 2021-11-14 19:41 ` Frank Rowand 2021-11-15 8:13 ` Matthias Schiffer 0 siblings, 1 reply; 6+ messages in thread From: Frank Rowand @ 2021-11-14 19:41 UTC (permalink / raw) To: Matthias Schiffer, Rob Herring; +Cc: devicetree, linux-kernel On 11/8/21 3:48 AM, Matthias Schiffer wrote: > Allow fully disabling CPU nodes using status = "fail". Having no status > property at all is still interpreted as "okay" as usual. > > This allows a bootloader to change the number of available CPUs (for > example when a common DTS is used for SoC variants with different numbers > of cores) without deleting the nodes altogether, which could require > additional fixups to avoid dangling phandle references. > > References: > - https://www.lkml.org/lkml/2020/8/26/1237 > - https://www.spinics.net/lists/devicetree-spec/msg01007.html > - https://github.com/devicetree-org/dt-schema/pull/61 > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/of/base.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index 61de453b885c..4e9973627c8d 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device) > } > EXPORT_SYMBOL(of_device_is_available); > > +/** > + * __of_device_is_disabled - check if a device has status "disabled" > + * > + * @device: Node to check status for, with locks already held > + * > + * Return: True if the status property is set to "disabled", > + * false otherwise > + * > + * Most callers should use __of_device_is_available() instead, this function > + * only exists due to the special interpretation of the "disabled" status for > + * CPU nodes. > + */ > +static bool __of_device_is_disabled(const struct device_node *device) > +{ > + const char *status; > + > + if (!device) > + return false; > + > + status = __of_get_property(device, "status", NULL); > + if (status == NULL) > + return false; > + > + return !strcmp(status, "disabled"); > +} > + > /** > * of_device_is_big_endian - check if a device has BE registers > * > @@ -817,6 +843,9 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) > of_node_put(node); > } > for (; next; next = next->sibling) { > + if (!__of_device_is_available(next) && > + !__of_device_is_disabled(next)) Shouldn't that just be a check to continue if the device is disabled? If adding a check for available, then all of the callers of for_each_of_cpu_node() need to be checked. There is at least one that is suspicious - arch/arm/mach-imx/platsmp.c has a comment: * Initialise the CPU possible map early - this describes the CPUs * which may be present or become present in the system. -Frank > + continue; > if (!(of_node_name_eq(next, "cpu") || > __of_node_is_type(next, "cpu"))) > continue; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status 2021-11-14 19:41 ` Frank Rowand @ 2021-11-15 8:13 ` Matthias Schiffer 2021-11-15 17:23 ` Frank Rowand 0 siblings, 1 reply; 6+ messages in thread From: Matthias Schiffer @ 2021-11-15 8:13 UTC (permalink / raw) To: Frank Rowand; +Cc: devicetree, linux-kernel, Rob Herring On Sun, 2021-11-14 at 14:41 -0500, Frank Rowand wrote: > On 11/8/21 3:48 AM, Matthias Schiffer wrote: > > Allow fully disabling CPU nodes using status = "fail". Having no status > > property at all is still interpreted as "okay" as usual. > > > > This allows a bootloader to change the number of available CPUs (for > > example when a common DTS is used for SoC variants with different numbers > > of cores) without deleting the nodes altogether, which could require > > additional fixups to avoid dangling phandle references. > > > > References: > > - https://www.lkml.org/lkml/2020/8/26/1237 > > - https://www.spinics.net/lists/devicetree-spec/msg01007.html > > - https://github.com/devicetree-org/dt-schema/pull/61 > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > --- > > drivers/of/base.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c > > index 61de453b885c..4e9973627c8d 100644 > > --- a/drivers/of/base.c > > +++ b/drivers/of/base.c > > @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device) > > } > > EXPORT_SYMBOL(of_device_is_available); > > > > +/** > > + * __of_device_is_disabled - check if a device has status "disabled" > > + * > > + * @device: Node to check status for, with locks already held > > + * > > + * Return: True if the status property is set to "disabled", > > + * false otherwise > > + * > > + * Most callers should use __of_device_is_available() instead, this function > > + * only exists due to the special interpretation of the "disabled" status for > > + * CPU nodes. > > + */ > > +static bool __of_device_is_disabled(const struct device_node *device) > > +{ > > + const char *status; > > + > > + if (!device) > > + return false; > > + > > + status = __of_get_property(device, "status", NULL); > > + if (status == NULL) > > + return false; > > + > > + return !strcmp(status, "disabled"); > > +} > > + > > /** > > * of_device_is_big_endian - check if a device has BE registers > > * > > @@ -817,6 +843,9 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) > > of_node_put(node); > > } > > for (; next; next = next->sibling) { > > + if (!__of_device_is_available(next) && > > + !__of_device_is_disabled(next)) > > Shouldn't that just be a check to continue if the device is disabled? > > If adding a check for available, then all of the callers of for_each_of_cpu_node() > need to be checked. There is at least one that is suspicious - arch/arm/mach-imx/platsmp.c > has a comment: > > * Initialise the CPU possible map early - this describes the CPUs > * which may be present or become present in the system. Previously, there were two option for the (effective) value of the status of a device_node: - "okay", "ok" or unset - anything else (which includes "disabled" and "fail") __of_device_is_available() checks which of these two is the case. With the new code, we have 3 cases for the status of CPU nodes: - "okay", "ok" or unset - "disabled" - anything else ("fail", ...) My patch will only change the behaviour in one case: When a CPU node has a status that is not "okay", "ok", "disabled" or unset - which is exactly the point of my patch. See also the change [1], which removed the !available check a while ago, and the discussion in [2], which led us to the conclusion that of_get_next_cpu_node() must not distinguish "okay" and "disabled" CPU nodes and we instead need a third status to disable a CPU for real. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/of/base.c?id=c961cb3be9064d1097ccc019390f8b5739daafc6 [2] https://www.lkml.org/lkml/2020/8/26/1237 > > -Frank > > > + continue; > > if (!(of_node_name_eq(next, "cpu") || > > __of_node_is_type(next, "cpu"))) > > continue; > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status 2021-11-15 8:13 ` Matthias Schiffer @ 2021-11-15 17:23 ` Frank Rowand 2021-11-16 8:32 ` Matthias Schiffer 0 siblings, 1 reply; 6+ messages in thread From: Frank Rowand @ 2021-11-15 17:23 UTC (permalink / raw) To: Matthias Schiffer; +Cc: devicetree, linux-kernel, Rob Herring Hi Matthias, On 11/15/21 3:13 AM, Matthias Schiffer wrote: > On Sun, 2021-11-14 at 14:41 -0500, Frank Rowand wrote: >> On 11/8/21 3:48 AM, Matthias Schiffer wrote: >>> Allow fully disabling CPU nodes using status = "fail". Having no status >>> property at all is still interpreted as "okay" as usual. >>> >>> This allows a bootloader to change the number of available CPUs (for >>> example when a common DTS is used for SoC variants with different numbers >>> of cores) without deleting the nodes altogether, which could require >>> additional fixups to avoid dangling phandle references. >>> >>> References: >>> - https://www.lkml.org/lkml/2020/8/26/1237 >>> - https://www.spinics.net/lists/devicetree-spec/msg01007.html >>> - https://github.com/devicetree-org/dt-schema/pull/61 >>> >>> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> >>> --- >>> drivers/of/base.c | 29 +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>> index 61de453b885c..4e9973627c8d 100644 >>> --- a/drivers/of/base.c >>> +++ b/drivers/of/base.c >>> @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device) >>> } >>> EXPORT_SYMBOL(of_device_is_available); >>> >>> +/** >>> + * __of_device_is_disabled - check if a device has status "disabled" >>> + * >>> + * @device: Node to check status for, with locks already held >>> + * >>> + * Return: True if the status property is set to "disabled", >>> + * false otherwise >>> + * >>> + * Most callers should use __of_device_is_available() instead, this function >>> + * only exists due to the special interpretation of the "disabled" status for >>> + * CPU nodes. >>> + */ >>> +static bool __of_device_is_disabled(const struct device_node *device) >>> +{ >>> + const char *status; >>> + >>> + if (!device) >>> + return false; >>> + >>> + status = __of_get_property(device, "status", NULL); >>> + if (status == NULL) >>> + return false; >>> + >>> + return !strcmp(status, "disabled"); >>> +} >>> + >>> /** >>> * of_device_is_big_endian - check if a device has BE registers >>> * >>> @@ -817,6 +843,9 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) >>> of_node_put(node); >>> } >>> for (; next; next = next->sibling) { >>> + if (!__of_device_is_available(next) && >>> + !__of_device_is_disabled(next)) >> >> Shouldn't that just be a check to continue if the device is disabled? >> >> If adding a check for available, then all of the callers of for_each_of_cpu_node() >> need to be checked. There is at least one that is suspicious - arch/arm/mach-imx/platsmp.c >> has a comment: >> >> * Initialise the CPU possible map early - this describes the CPUs >> * which may be present or become present in the system. > Thanks for the links to previous discussion you provided below. I had forgotten the previous discussion. In [2] Rob ended up saying that there were two options he was fine with. Either (or both), in of_get_next_cpu_node(), (1) use status of "fail" as the check or (2) use status of "disabled" as the check, conditional on !IS_ENABLED(CONFIG_PPC) "this would need some spec update" "Need to double check MIPS and Sparc though." Neither of these two options are what this patch does. It seems to me that option 1 is probably the easiest and least intrusive method. -Frank > Previously, there were two option for the (effective) value of the > status of a device_node: > > - "okay", "ok" or unset > - anything else (which includes "disabled" and "fail") > > __of_device_is_available() checks which of these two is the case. > > With the new code, we have 3 cases for the status of CPU nodes: > > - "okay", "ok" or unset > - "disabled" > - anything else ("fail", ...) > > My patch will only change the behaviour in one case: When a CPU node > has a status that is not "okay", "ok", "disabled" or unset - which is > exactly the point of my patch. > > See also the change [1], which removed the !available check a while > ago, and the discussion in [2], which led us to the conclusion that > of_get_next_cpu_node() must not distinguish "okay" and "disabled" CPU > nodes and we instead need a third status to disable a CPU for real. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/of/base.c?id=c961cb3be9064d1097ccc019390f8b5739daafc6 > [2] https://www.lkml.org/lkml/2020/8/26/1237 > > >> >> -Frank >> >>> + continue; >>> if (!(of_node_name_eq(next, "cpu") || >>> __of_node_is_type(next, "cpu"))) >>> continue; >>> >> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status 2021-11-15 17:23 ` Frank Rowand @ 2021-11-16 8:32 ` Matthias Schiffer 2021-11-16 15:07 ` Frank Rowand 0 siblings, 1 reply; 6+ messages in thread From: Matthias Schiffer @ 2021-11-16 8:32 UTC (permalink / raw) To: Frank Rowand; +Cc: devicetree, linux-kernel, Rob Herring On Mon, 2021-11-15 at 12:23 -0500, Frank Rowand wrote: > Hi Matthias, > > On 11/15/21 3:13 AM, Matthias Schiffer wrote: > > On Sun, 2021-11-14 at 14:41 -0500, Frank Rowand wrote: > > > On 11/8/21 3:48 AM, Matthias Schiffer wrote: > > > > Allow fully disabling CPU nodes using status = "fail". Having no status > > > > property at all is still interpreted as "okay" as usual. > > > > > > > > This allows a bootloader to change the number of available CPUs (for > > > > example when a common DTS is used for SoC variants with different numbers > > > > of cores) without deleting the nodes altogether, which could require > > > > additional fixups to avoid dangling phandle references. > > > > > > > > References: > > > > - https://www.lkml.org/lkml/2020/8/26/1237 > > > > - https://www.spinics.net/lists/devicetree-spec/msg01007.html > > > > - https://github.com/devicetree-org/dt-schema/pull/61 > > > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > > --- > > > > drivers/of/base.c | 29 +++++++++++++++++++++++++++++ > > > > 1 file changed, 29 insertions(+) > > > > > > > > diff --git a/drivers/of/base.c b/drivers/of/base.c > > > > index 61de453b885c..4e9973627c8d 100644 > > > > --- a/drivers/of/base.c > > > > +++ b/drivers/of/base.c > > > > @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device) > > > > } > > > > EXPORT_SYMBOL(of_device_is_available); > > > > > > > > +/** > > > > + * __of_device_is_disabled - check if a device has status "disabled" > > > > + * > > > > + * @device: Node to check status for, with locks already held > > > > + * > > > > + * Return: True if the status property is set to "disabled", > > > > + * false otherwise > > > > + * > > > > + * Most callers should use __of_device_is_available() instead, this function > > > > + * only exists due to the special interpretation of the "disabled" status for > > > > + * CPU nodes. > > > > + */ > > > > +static bool __of_device_is_disabled(const struct device_node *device) > > > > +{ > > > > + const char *status; > > > > + > > > > + if (!device) > > > > + return false; > > > > + > > > > + status = __of_get_property(device, "status", NULL); > > > > + if (status == NULL) > > > > + return false; > > > > + > > > > + return !strcmp(status, "disabled"); > > > > +} > > > > + > > > > /** > > > > * of_device_is_big_endian - check if a device has BE registers > > > > * > > > > @@ -817,6 +843,9 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) > > > > of_node_put(node); > > > > } > > > > for (; next; next = next->sibling) { > > > > + if (!__of_device_is_available(next) && > > > > + !__of_device_is_disabled(next)) > > > > > > Shouldn't that just be a check to continue if the device is disabled? > > > > > > If adding a check for available, then all of the callers of for_each_of_cpu_node() > > > need to be checked. There is at least one that is suspicious - arch/arm/mach-imx/platsmp.c > > > has a comment: > > > > > > * Initialise the CPU possible map early - this describes the CPUs > > > * which may be present or become present in the system. > > Thanks for the links to previous discussion you provided below. I had > forgotten the previous discussion. > > In [2] Rob ended up saying that there were two options he was fine with. > Either (or both), in of_get_next_cpu_node(), > > (1) use status of "fail" as the check or > > (2) use status of "disabled" as the check, conditional on !IS_ENABLED(CONFIG_PPC) > "this would need some spec update" > "Need to double check MIPS and Sparc though." > > Neither of these two options are what this patch does. It seems to me that > option 1 is probably the easiest and least intrusive method. My intuition is that a device with an unknown status value should not be used. For most devices this is already handled by treating any value that is not unset, "okay" or "ok" the same. For CPU nodes, this would be the case by treating such values like "fail". I did a quick grep through the in-tree Device Trees, and I did find a few unusual status properties (none of them in CPU nodes though): - Typo "failed" (4 cases) - Typo "disable" (17 cases) - "reserved" (19 cases) "fail" appears 2 times, the rest is "okay", "ok" or "disabled". I do not have a strong opinion on this though - for our concrete usecase, checking for "fail" is fine, and we can treat unknown values like "disabled" if you prefer that solution. Should "fail-*" status values also be treated like "fail" then? > > -Frank > > > Previously, there were two option for the (effective) value of the > > status of a device_node: > > > > - "okay", "ok" or unset > > - anything else (which includes "disabled" and "fail") > > > > __of_device_is_available() checks which of these two is the case. > > > > With the new code, we have 3 cases for the status of CPU nodes: > > > > - "okay", "ok" or unset > > - "disabled" > > - anything else ("fail", ...) > > > > My patch will only change the behaviour in one case: When a CPU node > > has a status that is not "okay", "ok", "disabled" or unset - which is > > exactly the point of my patch. > > > > See also the change [1], which removed the !available check a while > > ago, and the discussion in [2], which led us to the conclusion that > > of_get_next_cpu_node() must not distinguish "okay" and "disabled" CPU > > nodes and we instead need a third status to disable a CPU for real. > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/of/base.c?id=c961cb3be9064d1097ccc019390f8b5739daafc6 > > [2] https://www.lkml.org/lkml/2020/8/26/1237 > > > > > > > > > > -Frank > > > > > > > + continue; > > > > if (!(of_node_name_eq(next, "cpu") || > > > > __of_node_is_type(next, "cpu"))) > > > > continue; > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status 2021-11-16 8:32 ` Matthias Schiffer @ 2021-11-16 15:07 ` Frank Rowand 0 siblings, 0 replies; 6+ messages in thread From: Frank Rowand @ 2021-11-16 15:07 UTC (permalink / raw) To: Matthias Schiffer; +Cc: devicetree, linux-kernel, Rob Herring Hi Mathias, On 11/16/21 3:32 AM, Matthias Schiffer wrote: > On Mon, 2021-11-15 at 12:23 -0500, Frank Rowand wrote: >> Hi Matthias, >> >> On 11/15/21 3:13 AM, Matthias Schiffer wrote: >>> On Sun, 2021-11-14 at 14:41 -0500, Frank Rowand wrote: >>>> On 11/8/21 3:48 AM, Matthias Schiffer wrote: It turns out I confused myself a bit... The first email provided the clue that I needed: >>>>> Allow fully disabling CPU nodes using status = "fail". Having no status >>>>> property at all is still interpreted as "okay" as usual. I managed to forget that sentence before I looked at the code in the patch. >>>>> >>>>> This allows a bootloader to change the number of available CPUs (for >>>>> example when a common DTS is used for SoC variants with different numbers >>>>> of cores) without deleting the nodes altogether, which could require >>>>> additional fixups to avoid dangling phandle references. >>>>> >>>>> References: >>>>> - https://www.lkml.org/lkml/2020/8/26/1237 >>>>> - https://www.spinics.net/lists/devicetree-spec/msg01007.html >>>>> - https://github.com/devicetree-org/dt-schema/pull/61 >>>>> >>>>> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> >>>>> --- >>>>> drivers/of/base.c | 29 +++++++++++++++++++++++++++++ >>>>> 1 file changed, 29 insertions(+) >>>>> >>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>>>> index 61de453b885c..4e9973627c8d 100644 >>>>> --- a/drivers/of/base.c >>>>> +++ b/drivers/of/base.c >>>>> @@ -650,6 +650,32 @@ bool of_device_is_available(const struct device_node *device) >>>>> } >>>>> EXPORT_SYMBOL(of_device_is_available); >>>>> >>>>> +/** >>>>> + * __of_device_is_disabled - check if a device has status "disabled" >>>>> + * >>>>> + * @device: Node to check status for, with locks already held >>>>> + * >>>>> + * Return: True if the status property is set to "disabled", >>>>> + * false otherwise >>>>> + * >>>>> + * Most callers should use __of_device_is_available() instead, this function >>>>> + * only exists due to the special interpretation of the "disabled" status for >>>>> + * CPU nodes. >>>>> + */ >>>>> +static bool __of_device_is_disabled(const struct device_node *device) >>>>> +{ >>>>> + const char *status; >>>>> + >>>>> + if (!device) >>>>> + return false; >>>>> + >>>>> + status = __of_get_property(device, "status", NULL); >>>>> + if (status == NULL) >>>>> + return false; >>>>> + >>>>> + return !strcmp(status, "disabled"); >>>>> +} >>>>> + >>>>> /** >>>>> * of_device_is_big_endian - check if a device has BE registers >>>>> * >>>>> @@ -817,6 +843,9 @@ struct device_node *of_get_next_cpu_node(struct device_node *prev) >>>>> of_node_put(node); >>>>> } So in the following test: >>>>> for (; next; next = next->sibling) { >>>>> + if (!__of_device_is_available(next) && >>>>> + !__of_device_is_disabled(next)) (!__of_device_is_available(next) && !__of_device_is_disabled(next)) is meant to detect a status of "fail" or "fail-sss". Since I forget the sentence about "fail" from early in the email, I had difficulty in interpreting the intent of the (!ok && !disabled) form of the test. The intent of the test would be more clear if it was checking _for_ "fail" instead of checking for _not_ the other possible status values. So you _are_ checking for the status of "fail" check (Rob's choice #1 below) and I just did not understand that was the intent of the patch. So I am fine with the patch if you change the above logic to check for "fail" or "fail-sss". It would also be good to add a comment to the of_get_next_cpu_node() header comment that "fail" nodes are excluded. Sorry for the confusion. -Frank >>>> >>>> Shouldn't that just be a check to continue if the device is disabled? >>>> >>>> If adding a check for available, then all of the callers of for_each_of_cpu_node() >>>> need to be checked. There is at least one that is suspicious - arch/arm/mach-imx/platsmp.c >>>> has a comment: >>>> >>>> * Initialise the CPU possible map early - this describes the CPUs >>>> * which may be present or become present in the system. >> >> Thanks for the links to previous discussion you provided below. I had >> forgotten the previous discussion. >> >> In [2] Rob ended up saying that there were two options he was fine with. >> Either (or both), in of_get_next_cpu_node(), >> >> (1) use status of "fail" as the check or >> >> (2) use status of "disabled" as the check, conditional on !IS_ENABLED(CONFIG_PPC) >> "this would need some spec update" >> "Need to double check MIPS and Sparc though." >> >> Neither of these two options are what this patch does. It seems to me that >> option 1 is probably the easiest and least intrusive method. > > My intuition is that a device with an unknown status value should not > be used. For most devices this is already handled by treating any value > that is not unset, "okay" or "ok" the same. For CPU nodes, this would > be the case by treating such values like "fail". > > I did a quick grep through the in-tree Device Trees, and I did find a > few unusual status properties (none of them in CPU nodes though): > > - Typo "failed" (4 cases) > - Typo "disable" (17 cases) > - "reserved" (19 cases) > > "fail" appears 2 times, the rest is "okay", "ok" or "disabled". > > I do not have a strong opinion on this though - for our concrete > usecase, checking for "fail" is fine, and we can treat unknown values > like "disabled" if you prefer that solution. Should "fail-*" status > values also be treated like "fail" then? > > > > >> >> -Frank >> >>> Previously, there were two option for the (effective) value of the >>> status of a device_node: >>> >>> - "okay", "ok" or unset >>> - anything else (which includes "disabled" and "fail") >>> >>> __of_device_is_available() checks which of these two is the case. >>> >>> With the new code, we have 3 cases for the status of CPU nodes: >>> >>> - "okay", "ok" or unset >>> - "disabled" >>> - anything else ("fail", ...) >>> >>> My patch will only change the behaviour in one case: When a CPU node >>> has a status that is not "okay", "ok", "disabled" or unset - which is >>> exactly the point of my patch. >>> >>> See also the change [1], which removed the !available check a while >>> ago, and the discussion in [2], which led us to the conclusion that >>> of_get_next_cpu_node() must not distinguish "okay" and "disabled" CPU >>> nodes and we instead need a third status to disable a CPU for real. >>> >>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/of/base.c?id=c961cb3be9064d1097ccc019390f8b5739daafc6 >>> [2] https://www.lkml.org/lkml/2020/8/26/1237 >>> >>> >>>> >>>> -Frank >>>> >>>>> + continue; >>>>> if (!(of_node_name_eq(next, "cpu") || >>>>> __of_node_is_type(next, "cpu"))) >>>>> continue; >>>>> >>>> >>>> >> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-16 15:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-08 8:48 [PATCH] of: base: Skip CPU nodes with non-"okay"/"disabled" status Matthias Schiffer 2021-11-14 19:41 ` Frank Rowand 2021-11-15 8:13 ` Matthias Schiffer 2021-11-15 17:23 ` Frank Rowand 2021-11-16 8:32 ` Matthias Schiffer 2021-11-16 15:07 ` Frank Rowand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).