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

[Simon Glass]

Hi Marek,

On Thu, 20 Nov 2025 at 21:39, Marek Vasut <marek.vasut at mailbox.org> wrote:
>
> On 11/21/25 12:40 AM, Simon Glass wrote:
> > Hi Marek,
> >
> > On Thu, 20 Nov 2025 at 04:56, Marek Vasut <marek.vasut at mailbox.org> wrote:
> >>
> >> On 11/20/25 10:08 AM, Heiko Schocher wrote:
> >>
> >> Hello Heiko,
> >>
> >>>> 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 ?
> >> No, this was the previous implementation which is now superseded by this
> >> one, see [1] [2]. The goal is to fix the entry points which pass invalid
> >> content further into U-Boot DT handling code, so the DT validation
> >> happens only one, on DT input, and not again later.
> >
> > Do the libfdt functions work correctly with *any* negative numbers?
>
> Not without a full tree check (LIBFDT_ASSUME_MASK=0x00). With
> LIBFDT_ASSUME_MASK=0xff (what SPL uses, tree checks disabled), passing
> negative offset to libfdt functions would simply trigger a segfault for
> sandbox SPL tests.
>
> > Do
> > you have a pointer to the libfdt commit which changed this? All of
> > driver model relies on that behaviour in libfdt:
> With libfdt 1.7.2 , this code triggers segfault in sandbox SPL tests:
>
> 162 uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> 163 {
> 164         const fdt32_t *tagp, *lenp;
> 165         uint32_t tag, len, sum;
> 166         int offset = startoffset;
> 167         const char *p;
> 168
> 169         *nextoffset = -FDT_ERR_TRUNCATED;
> 170         tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> 171         if (!can_assume(VALID_DTB) && !tagp)
> 172                 return FDT_END; /* premature end */
> 173         tag = fdt32_to_cpu(*tagp);
>
> The fdt_offset_ptr() returns NULL and fdt32_to_cpu(*tagp) would
> dereference that NULL pointer.

Ah, I see. So the library hasn't changed, it's just the 'assume'
logic. That makes sense.

I see a few options:
- enable the checks for the affected board (code-size impact)
- create a new VALID_OFFSET assumption (split out from VALID_DTB)
which checks offsets in fdt_next_tag()

The latter might be best. It would likely be very cheap in terms of
code size. In fact, I should have thought of this at the time.

Regards,
Simon