raspi: should vc fixups update properties of existing nodes?

raspi: should vc fixups update properties of existing nodes?

[Jonas Richardsen]

Hi,

the function `rpi_vc_fdt_parse` in 
arch/arm/boards/raspberry-pi/rpi-common.c registers multiple fixups that 
take over nodes from the video core device tree. These fixups make use 
of the `of_copy_property` function to copy the properties of the 
respective node. In the case of already existing nodes (and properties), 
this function duplicates the properties instead of updating them.

If the intention is to not override existing properties, one should 
probably check for the existence of each property before copying to 
avoid kernel warnings and misconfiguration due to duplicate properties.

If existing properties are supposed to be updated, this could be 
achieved by switching to `of_set_property` (or something similar). Note 
that this notion of overriding properties also exists in video core, see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/broadcom/bcm2711.dtsi?h=v6.8#n412 
for an example.

I would be glad to submit a patch for either case.

Regards,

Jonas

Re: raspi: should vc fixups update properties of existing nodes?

[Ahmad Fatoum]

Hello Jonas,

On 10.04.24 11:59, Jonas Richardsen wrote:
> Hi,
> 
> the function `rpi_vc_fdt_parse` in arch/arm/boards/raspberry-pi/rpi-common.c registers multiple fixups that take over nodes from the video core device tree. These fixups make use of the `of_copy_property` function to copy the properties of the respective node. In the case of already existing nodes (and properties), this function duplicates the properties instead of updating them.

Ouch.

> If the intention is to not override existing properties, one should probably check for the existence of each property before copying to avoid kernel warnings and misconfiguration due to duplicate properties.

I think that's the way to go. of_copy_property should maybe return
PTR_ERR(-EEXIST) if the property already exists. Users are free to
use of_delete_property_by_name beforehand if they want to remove
the content.

> If existing properties are supposed to be updated, this could be achieved by switching to `of_set_property` (or something similar). Note that this notion of overriding properties also exists in video core, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/broadcom/bcm2711.dtsi?h=v6.8#n412 for an example.

I think the default should be not to override and exceptions should
be done explicitly.

> I would be glad to submit a patch for either case.

That would be great!

Thanks,
Ahmad

> 
> Regards,
> 
> Jonas
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] of: do not copy properties if they already exist in the destination

[Jonas Richardsen]

---
 drivers/of/base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b22959dabe..9bb4ae13db 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2346,6 +2346,9 @@ struct property *of_copy_property(const struct device_node *src,
 	if (!prop)
 		return NULL;
 
+	if (of_find_property(dst, propname, NULL))
+		return ERR_PTR(-EEXIST);
+
 	return of_new_property(dst, propname,
 			       of_property_get_value(prop), prop->length);
 }
-- 
2.42.0

Re: [PATCH] of: do not copy properties if they already exist in the destination

[Ahmad Fatoum]

On 15.04.24 10:14, Jonas Richardsen wrote:

Please write a short sentence here, justifying the change and add
you S-o-b, see https://developercertificate.org/

> ---
>  drivers/of/base.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index b22959dabe..9bb4ae13db 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2346,6 +2346,9 @@ struct property *of_copy_property(const struct device_node *src,
>  	if (!prop)
>  		return NULL;
>  
> +	if (of_find_property(dst, propname, NULL))
> +		return ERR_PTR(-EEXIST);

Use of_property_present() here instead.

Cheers,
Ahmad

> +
>  	return of_new_property(dst, propname,
>  			       of_property_get_value(prop), prop->length);
>  }

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2] of: do not copy properties if they already exist in the destination

[Jonas Richardsen]

Currently `of_copy_property` copies the given property even if a property 
with the same name already exists on the destination node. 
This leads to kernel warnings about duplicate properties:
```
[    0.014063] Duplicate name in chosen, renamed to "stdout-path#1"
[    0.014093] Duplicate name in chosen, renamed to "bootargs#1"
[    0.014119] Duplicate name in chosen, renamed to "phandle#1"
[    0.014197] Duplicate name in reserved-memory, renamed to "#address-cells#1"
[    0.014226] Duplicate name in reserved-memory, renamed to "#size-cells#1"
[    0.014252] Duplicate name in reserved-memory, renamed to "ranges#1"
[    0.014278] Duplicate name in reserved-memory, renamed to "phandle#1"
```
Therefore, the function was changed to return an error if the property
already exists in the destination.
The change does not cause any regressions, because the only usage of
this function occurs within `arch/arm/boards/raspberry-pi/rpi-common.c`
where the original behaviour of the function is obviously unintended.

Signed-off-by: Jonas Richardsen <jonasrichardsen@emlix.com>
---
In the process of creating this patch, we realized that the fixups 
    for the Raspberry Pi are extensively copying properties from the video
    core device tree that are not strictly necessary. We will do some
    work on making the fixups more precise (i.e., only copy specific
    properties, not entire nodes) soon.

 drivers/of/base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b22959dabe..3192a74ab9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2346,6 +2346,9 @@ struct property *of_copy_property(const struct device_node *src,
 	if (!prop)
 		return NULL;
 
+	if (of_property_present(dst, propname))
+		return ERR_PTR(-EEXIST);
+
 	return of_new_property(dst, propname,
 			       of_property_get_value(prop), prop->length);
 }
-- 
2.42.0

Re: [PATCH v2] of: do not copy properties if they already exist in the destination

[Sascha Hauer]

On Mon, 15 Apr 2024 14:26:04 +0200, Jonas Richardsen wrote:
> Currently `of_copy_property` copies the given property even if a property
> with the same name already exists on the destination node.
> This leads to kernel warnings about duplicate properties:
> ```
> [    0.014063] Duplicate name in chosen, renamed to "stdout-path#1"
> [    0.014093] Duplicate name in chosen, renamed to "bootargs#1"
> [    0.014119] Duplicate name in chosen, renamed to "phandle#1"
> [    0.014197] Duplicate name in reserved-memory, renamed to "#address-cells#1"
> [    0.014226] Duplicate name in reserved-memory, renamed to "#size-cells#1"
> [    0.014252] Duplicate name in reserved-memory, renamed to "ranges#1"
> [    0.014278] Duplicate name in reserved-memory, renamed to "phandle#1"
> ```
> Therefore, the function was changed to return an error if the property
> already exists in the destination.
> The change does not cause any regressions, because the only usage of
> this function occurs within `arch/arm/boards/raspberry-pi/rpi-common.c`
> where the original behaviour of the function is obviously unintended.
> 
> [...]

Applied, thanks!

[1/1] of: do not copy properties if they already exist in the destination
      https://git.pengutronix.de/cgit/barebox/commit/?id=364a1831678d (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>