[PATCH 0/2] of: add support for value "false" to of_property_read_bool

[PATCH 0/2] of: add support for value "false" to of_property_read_bool

[Josua Mayer]

Boolean type properties are usually considered true if present and false
when they do not exist. This works well for many in-tree board dts and
existing drivers.
    
When users need to overrride boolean values from included dts,
/delete-property/ is recommend. This however does not work in overlays
(addons).

Geert pointed out [1] that there are several invitations for using
strings "true" and "false" on boolean properties: [1], [2], [3].

Add support for a string value "false" to be considered false on boolean
properties by changing of_property_read_bool implementation.

Affected by this issue is AM642 HummingBoard-T overlay to enable usb-3.1
function. This is an alternative solution to dropping
it's disfynctional overlay [1].

[1] https://lore.kernel.org/linux-devicetree/CAMuHMdWY0J7uooeRbUGwjkeCLd2UVyN9dtDWUkg5pJ3sAULdsQ@mail.gmail.com/
[2] Documentation/devicetree/bindings/sound/rt5651.txt
[3] Documentation/devicetree/bindings/sound/pcm3060.txt
[4] arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi
[5] https://lore.kernel.org/linux-devicetree/20241101-am64-hb-fix-overlay-v1-1-080b98b057b6@solid-run.com/

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Closes: https://lore.kernel.org/linux-devicetree/CAMuHMdXTgpTnJ9U7egC2XjFXXNZ5uiY1O+WxNd6LPJW5Rs5KTw@mail.gmail.com
Fixes: bbef42084cc1 ("arm64: dts: ti: hummingboard-t: add overlays for m.2 pci-e and usb-3")
Signed-off-by: Josua Mayer <josua@solid-run.com>

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Jon Nettleton <jon@solid-run.com>
Cc: Yazan Shhady <yazan.shhady@solid-run.com>
Cc: rabeeh@solid-run.com

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Josua Mayer (2):
      of: add support for value "false" to of_property_read_bool
      arm64: dts: ti: k3-am642-hummingboard-t-usb3: fix overlay boolean value

 arch/arm64/boot/dts/ti/k3-am642-hummingboard-t-usb3.dtso |  2 +-
 drivers/of/property.c                                    |  2 +-
 include/linux/of.h                                       | 13 ++++++++-----
 3 files changed, 10 insertions(+), 7 deletions(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241112-am64-overlay-bool-60a35da933f7

Best regards,
-- 
Josua Mayer <josua@solid-run.com>

[PATCH 1/2] of: add support for value "false" to of_property_read_bool

[Josua Mayer]

Boolean type properties are usually considered true if present and false
when they do not exist. This works well for many in-tree board dts and
existing drivers.

When users need to overrride boolean values from included dts includes,
/delete-property/ is recommend. This however does not work in overlays
(addons).

Several places use string "true" ([1], [2], [3]) and one uses string
"false" ([1]). This suggests that at some point the value of a type
string property was to be taken into account during evaluation.

Change of_property_read_bool to only return true if a property is both
present, and not equal "false".

Existing usage in drivers/of and include/linux/of.h are updated
accordingly.
Other places should be reviewed as needed wrt. changed semantics.

[1] Documentation/devicetree/bindings/sound/rt5651.txt
[2] Documentation/devicetree/bindings/sound/pcm3060.txt
[3] arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 drivers/of/property.c |  2 +-
 include/linux/of.h    | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 11b922fde7af167cc69f6dd89826219653cd1ee8..a51132b883fb3fa0b1a120bacb298abf72f67c5b 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -893,7 +893,7 @@ of_fwnode_device_get_dma_attr(const struct fwnode_handle *fwnode)
 static bool of_fwnode_property_present(const struct fwnode_handle *fwnode,
 				       const char *propname)
 {
-	return of_property_read_bool(to_of_node(fwnode), propname);
+	return of_property_present(to_of_node(fwnode), propname);
 }
 
 static int of_fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
diff --git a/include/linux/of.h b/include/linux/of.h
index 85b60ac9eec50bb202aa774cab273e1d947e394d..ea65c7d1176194d6ce79432782361f2cdab84f8b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1247,14 +1247,15 @@ static inline int of_property_read_string_index(const struct device_node *np,
  * Search for a boolean property in a device node. Usage on non-boolean
  * property types is deprecated.
  *
- * Return: true if the property exists false otherwise.
+ * Return: true if the property exists and value is not "false",
+ * false otherwise.
  */
 static inline bool of_property_read_bool(const struct device_node *np,
 					 const char *propname)
 {
-	struct property *prop = of_find_property(np, propname, NULL);
+	int ret = of_property_match_string(np, propname, "false");
 
-	return prop ? true : false;
+	return ret == -ENODATA ? true : false;
 }
 
 /**
@@ -1268,7 +1269,9 @@ static inline bool of_property_read_bool(const struct device_node *np,
  */
 static inline bool of_property_present(const struct device_node *np, const char *propname)
 {
-	return of_property_read_bool(np, propname);
+	struct property *prop = of_find_property(np, propname, NULL);
+
+	return prop ? true : false;
 }
 
 /**
@@ -1679,7 +1682,7 @@ static inline int of_reconfig_get_state_change(unsigned long action,
  * of_device_is_system_power_controller - Tells if system-power-controller is found for device_node
  * @np: Pointer to the given device_node
  *
- * Return: true if present false otherwise
+ * Return: true if enabled, false otherwise
  */
 static inline bool of_device_is_system_power_controller(const struct device_node *np)
 {

-- 
2.43.0

[PATCH 2/2] arm64: dts: ti: k3-am642-hummingboard-t-usb3: fix overlay boolean value

[Josua Mayer]

The USB-3.1 overlay uses /delete-property/ to unset a boolean property
on the usb controller limiting it to USB-2.0 by default.
Overlays can not delete a property from the base dtb.

Change the overlay to instead assign value "false" on the ti,usb2-only
boolean property, which is of_property_read_bool now evaluates as false.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Closes: https://lore.kernel.org/linux-devicetree/CAMuHMdXTgpTnJ9U7egC2XjFXXNZ5uiY1O+WxNd6LPJW5Rs5KTw@mail.gmail.com
Fixes: bbef42084cc1 ("arm64: dts: ti: hummingboard-t: add overlays for m.2 pci-e and usb-3")
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 arch/arm64/boot/dts/ti/k3-am642-hummingboard-t-usb3.dtso | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am642-hummingboard-t-usb3.dtso b/arch/arm64/boot/dts/ti/k3-am642-hummingboard-t-usb3.dtso
index ffcc3bd3c7bc5d47ce9926a95a13af3f61182a2b..3ed709ff11fea1ba07e180d442ce6fda6acaa2a3 100644
--- a/arch/arm64/boot/dts/ti/k3-am642-hummingboard-t-usb3.dtso
+++ b/arch/arm64/boot/dts/ti/k3-am642-hummingboard-t-usb3.dtso
@@ -34,7 +34,7 @@ &serdes_mux {
 };
 
 &usbss0 {
-	/delete-property/ ti,usb2-only;
+	ti,usb2-only = "false";
 };
 
 &usb0 {

-- 
2.43.0

Re: [PATCH 1/2] of: add support for value "false" to of_property_read_bool

[Josua Mayer]

Am 12.11.24 um 08:41 schrieb Josua Mayer:
> Boolean type properties are usually considered true if present and false
> when they do not exist. This works well for many in-tree board dts and
> existing drivers.
>
> When users need to overrride boolean values from included dts includes,
> /delete-property/ is recommend. This however does not work in overlays
> (addons).
>
> Several places use string "true" ([1], [2], [3]) and one uses string
> "false" ([1]). This suggests that at some point the value of a type
> string property was to be taken into account during evaluation.
>
> Change of_property_read_bool to only return true if a property is both
> present, and not equal "false".
>
> Existing usage in drivers/of and include/linux/of.h are updated
> accordingly.
> Other places should be reviewed as needed wrt. changed semantics.
>
> [1] Documentation/devicetree/bindings/sound/rt5651.txt
> [2] Documentation/devicetree/bindings/sound/pcm3060.txt
> [3] arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi
>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
Please note this patch-set has only been tested to compile,
I do not consider it ready for current merge window.

I would however appreciate comments.

Re: [PATCH 0/2] of: add support for value "false" to of_property_read_bool

[Rob Herring]

On Tue, Nov 12, 2024 at 12:41 AM Josua Mayer <josua@solid-run.com> wrote:
>
> Boolean type properties are usually considered true if present and false
> when they do not exist. This works well for many in-tree board dts and
> existing drivers.
>
> When users need to overrride boolean values from included dts,
> /delete-property/ is recommend. This however does not work in overlays
> (addons).

As soon as someone needs to delete a non-boolean property, we're back
to the same problem. If you want to fix it, you need to fix it for any
property.


> Geert pointed out [1] that there are several invitations for using
> strings "true" and "false" on boolean properties: [1], [2], [3].

There's always bad examples...

> Add support for a string value "false" to be considered false on boolean
> properties by changing of_property_read_bool implementation.

Any existing s/w will treat 'foo = "false"' as true. It's an ABI.

Rob

Re: [PATCH 0/2] of: add support for value "false" to of_property_read_bool

[Josua Mayer]

Am 12.11.24 um 14:46 schrieb Rob Herring:
> On Tue, Nov 12, 2024 at 12:41 AM Josua Mayer <josua@solid-run.com> wrote:
>> Boolean type properties are usually considered true if present and false
>> when they do not exist. This works well for many in-tree board dts and
>> existing drivers.
>>
>> When users need to overrride boolean values from included dts,
>> /delete-property/ is recommend. This however does not work in overlays
>> (addons).
> As soon as someone needs to delete a non-boolean property, we're back
> to the same problem.

Properties can be reassigned any value, e.g. a driver default if needed.
It should never be necessary to delete a property,
since a suitable value may be specified instead.

Booleans have two valid values, true and false,
but somehow we cannot assign false, we can just delete the property.

> If you want to fix it, you need to fix it for any
> property.
/delete-property/ is a language keyword used during compilation.
When inspecting a .dtbo no trace of /delete-property/ is left.

Hence we can't "fix" deleting properties through overlays.
We can only "fix" (re-)assigning false to a boolean property.

>
>
>> Geert pointed out [1] that there are several invitations for using
>> strings "true" and "false" on boolean properties: [1], [2], [3].
> There's always bad examples...
>
>> Add support for a string value "false" to be considered false on boolean
>> properties by changing of_property_read_bool implementation.
> Any existing s/w will treat 'foo = "false"' as true. It's an ABI.
I was reading through the device-tree specification, it makes absolutely
no mention of a boolean type.

I believe of_property_read_bool should be capable of deriving false
from a present property.

What is up to now called bool in the kernel / device-tree,
is actually of_property_present, or conversationally of_node_has_flag.


sincerely
Josua Mayer

Re: [PATCH 0/2] of: add support for value "false" to of_property_read_bool

[Geert Uytterhoeven]

Hi Josua,

On Wed, Nov 13, 2024 at 10:41 AM Josua Mayer <josua@solid-run.com> wrote:
> Am 12.11.24 um 14:46 schrieb Rob Herring:
> > On Tue, Nov 12, 2024 at 12:41 AM Josua Mayer <josua@solid-run.com> wrote:
> >> Boolean type properties are usually considered true if present and false
> >> when they do not exist. This works well for many in-tree board dts and
> >> existing drivers.
> >>
> >> When users need to overrride boolean values from included dts,
> >> /delete-property/ is recommend. This however does not work in overlays
> >> (addons).
> > As soon as someone needs to delete a non-boolean property, we're back
> > to the same problem.
>
> Properties can be reassigned any value, e.g. a driver default if needed.
> It should never be necessary to delete a property,
> since a suitable value may be specified instead.
>
> Booleans have two valid values, true and false,
> but somehow we cannot assign false, we can just delete the property.
>
> > If you want to fix it, you need to fix it for any
> > property.
> /delete-property/ is a language keyword used during compilation.
> When inspecting a .dtbo no trace of /delete-property/ is left.

That is the current behavior. But dtc could be modified to emit new
special __deleted_properties__ and __deleted_nodes__ nodes, just like
it already creates special __symbols__, __fixups__, and __local_fixups__
nodes.

> Hence we can't "fix" deleting properties through overlays.
> We can only "fix" (re-)assigning false to a boolean property.

So nothing prevents adding a way for a .dtbo to contain a list of
properties and/or nodes to delete...

> >> Geert pointed out [1] that there are several invitations for using
> >> strings "true" and "false" on boolean properties: [1], [2], [3].
>
> > There's always bad examples...
> >
> >> Add support for a string value "false" to be considered false on boolean
> >> properties by changing of_property_read_bool implementation.
>
> > Any existing s/w will treat 'foo = "false"' as true. It's an ABI.
>
> I was reading through the device-tree specification, it makes absolutely
> no mention of a boolean type.
>
> I believe of_property_read_bool should be capable of deriving false
> from a present property.
>
> What is up to now called bool in the kernel / device-tree,
> is actually of_property_present, or conversationally of_node_has_flag.

Indeed, so of_property_read_bool() is a misnomer, as it does
not read the actual value from the property, unlike all other
of_property_read_*() methods.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds