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

[Simon Glass]

Hi Marek,

On Fri, 21 Nov 2025 at 07:52, Marek Vasut <marek.vasut at mailbox.org> wrote:
>
> On 11/21/25 3:26 PM, Simon Glass wrote:
> > 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)
>
> Every system that enables OF_PLATDATA is affected ?
>
> > - 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.
> Wouldn't it be better to not misuse libfdt ?

It actually isn't a misuse. It is perfectly OK to pass an invalid
offset and libfdt has defined behaviour in that case, absent any
'assumptions' we force.

Remember, in Linux these checks are enabled. We added the 'assume'
thing to reduce code size. Here you have found a situation where it
breaks stuff, so we should back off the assumption a bit.

Regards,
Simon