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

Message

Josua Mayer Nov. 12, 2024, 6:41 a.m. UTC

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/[e-mail]/
[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/[e-mail]/

Reported-by: Geert Uytterhoeven <[e-mail]>
Closes: https://lore.kernel.org/linux-devicetree/[e-mail]
Fixes: bbef42084cc1 ("arm64: dts: ti: hummingboard-t: add overlays for m.2 pci-e and usb-3")
Signed-off-by: Josua Mayer <[e-mail]>

Cc: Geert Uytterhoeven <[e-mail]>
Cc: Jon Nettleton <[e-mail]>
Cc: Yazan Shhady <[e-mail]>
Cc: [e-mail]

Signed-off-by: Josua Mayer <[e-mail]>
---
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,

Comments

Rob Herring (Arm) Nov. 12, 2024, 1:46 p.m. UTC | #1

On Tue, Nov 12, 2024 at 12:41 AM Josua Mayer <[e-mail]> 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

Josua Mayer Nov. 13, 2024, 9:41 a.m. UTC | #2

Am 12.11.24 um 14:46 schrieb Rob Herring:
> On Tue, Nov 12, 2024 at 12:41 AM Josua Mayer <[e-mail]> 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

Geert Uytterhoeven Nov. 13, 2024, 10:15 a.m. UTC | #3

Hi Josua,

On Wed, Nov 13, 2024 at 10:41 AM Josua Mayer <[e-mail]> wrote:
> Am 12.11.24 um 14:46 schrieb Rob Herring:
> > On Tue, Nov 12, 2024 at 12:41 AM Josua Mayer <[e-mail]> 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