[PATCH] i2c: sandbox: Avoid calling dev_read_*() if CONFIG_OF_PLATDATA=y

[Heiko Schocher]

Hi Marek,

On 20.11.25 05:15, Marek Vasut wrote:
> If CONFIG_OF_PLATDATA=y , then the udevice has no valid OF node associated
> with it and ofnode_valid(node) evaluates to 0. The dev_read_u32_default()
> call ultimately reaches ofnode_read_u32_index() which invokes fdt_getprop()
> and passes result of ofnode_to_offset(node) as an offset parameter into it.
> 
> The ofnode_to_offset(node) returns -1 for invalid node, which leads to an
> fdt_getprop(..., -1, ...) invocation, which will crash sandbox with SIGSEGV
> because libfdt can not handle negative node offsets without full tree check,
> which U-Boot inhibits to keep size lower.
> 
> Since i2c_child_post_bind() already calls dev_has_ofnode(dev), reuse the
> same call and assign i2c->speed_hz = I2C_SPEED_STANDARD_RATE in case the
> device has no valid node associated with it, and do not call any of the
> dev_read_*() functions for devices without valid nodes.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas at mailbox.org>
> ---
> Cc: Heiko Schocher <hs at nabladev.com>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: u-boot at lists.denx.de
> ---
>   drivers/i2c/i2c-uclass.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> index 380a9f8f3ad..d0e9dad7202 100644
> --- a/drivers/i2c/i2c-uclass.c
> +++ b/drivers/i2c/i2c-uclass.c
> @@ -749,8 +749,11 @@ static int i2c_post_probe(struct udevice *dev)
>   #if CONFIG_IS_ENABLED(OF_REAL)
>   	struct dm_i2c_bus *i2c = dev_get_uclass_priv(dev);
>   
> -	i2c->speed_hz = dev_read_u32_default(dev, "clock-frequency",
> -					     I2C_SPEED_STANDARD_RATE);
> +	if (!dev_has_ofnode(dev))
> +		i2c->speed_hz = I2C_SPEED_STANDARD_RATE;
> +	else
> +		i2c->speed_hz = dev_read_u32_default(dev, "clock-frequency",
> +						     I2C_SPEED_STANDARD_RATE);
>   
>   	return dm_i2c_set_bus_speed(dev, i2c->speed_hz);
>   #else
> 

This sounds more as a bug of dev_read_u32_default() to me and should
be fixed there, else we need to make this check around each call of
dev_read_u32_default() ...

And looking into u-boot:/drivers/core/read.c there are more
candidates for such a fix (all with "default" in function name?)

and such a fix would also make the dev_has_ofnode(dev) check in
i2c_child_post_bind() obsolete.

Or may we fix the default functions in drivers/core/ofnode.c ?

Something like that:
"""
hs at threadripper:u-boot  [master] $ git diff
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index cf1cf8abfbe..400512c1a30 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -401,7 +401,9 @@ int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)

  u32 ofnode_read_u32_default(ofnode node, const char *propname, u32 def)
  {
-       assert(ofnode_valid(node));
+       if (!ofnode_valid(node))
+               return def;
+
         ofnode_read_u32_index(node, propname, 0, &def);

         return def;
"""

I would interprete a default function in a way, that whatever happens,
it returns the default value... but may I oversee something obvious?

bye,
Heiko
-- 
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office at nabladev.com
Geschäftsführer : Stefano Babic