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

[Simon Glass]

Hi Tom,

On Fri, 21 Nov 2025 at 12:24, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Nov 21, 2025 at 12:09:21PM -0700, Simon Glass wrote:
> > Hi Marek,
> >
> > On Fri, 21 Nov 2025 at 11:01, Marek Vasut <marek.vasut at mailbox.org> wrote:
> > >
> > > On 11/21/25 6:40 PM, Simon Glass wrote:
> > >
> > > Hello Simon,
> > >
> > > >>>>> - 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.
> > > >>
> > > >> It actually isn't OK to pass in invalid offset if we compile FDT with
> > > >> disabled tree validity checks.
> > > >
> > > > We are saying exactly the same thing, if you read both of the above :-)
> > >
> > > What I wrote is, that it is NOT OK to pass invalid offset to libfdt IFF
> > > the DT validity checks are disabled. This is yes a misuse.
> >
> > Yes, of course. It IS OK if you enable the validity checks.
> >
> > >
> > > >>> 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.
> > > >> U-Boot also has those checks enabled, SPL does not, and things break
> > > >> when OF_PLATDATA is enabled and there is no valid node associated with
> > > >> the udevice.
> > > >
> > > > Right, so add SPL_OF_LIBFDT_ASSUME_MASK for SPL and check the code
> > > > size. If it is too much, add a new level FDT_ASSUME_EXISTS before
> > > > FDT_ASSUME_SANE and use that for the necessary checks (there will only
> > > > be a few, from my reading of the code).
> > > The SPL assumes the FDT is valid, and disables FDT validation in libfdt,
> > > to keep the code size low.
> > >
> > > U-Boot does enable that validation in libfdt, and grows its size a bit
> > > because of that (and also because the DT might come from outside), but
> > > SPL can not do that (and control DT does not generally come from outside
> > > in SPL).
> > >
> > > That is why these few fixes actually repair the misuse, instead of
> > > re-enabling the validation in libfdt.
> >
> > Enable basic validation if !OF_REAL ? Create a new level of 'assume'
> > for just what you need?
> >
> > It just seems really odd to disable validation and then try to not
> > call the functions in case they will crash. The next person who comes
> > along will have an unexplained crash. It's just too brittle.
>
> You seem to have missed the entire context of the problem here. Today,
> in upstream exactly one platform disables validation in full U-Boot.
> Exactly zero platforms enable any form of validation in xPL. Upstream
> dtc has made a large number of changes since v1.4.6. Whatever we have
> been assuming could be done cannot be done anymore.

I took a look at what I landed in upstream dtc, we have
ASSUME_VALID_DTB, so we should just use that.

It has very few checks. If we want one more step that really just
checks that fdt is not NULL, I could try to do that upstream.

As to my missing something, I am aware that we don't enable validation
in xPL (I still recall writing that code), but also that with
OF_PLATDATA enabled, we need some sort of basic validation if people
want to call dev_read_...() functions. My attempt to show that was to
add assert() statements in some of the ofnode functions. As part of
changing this, we should really remove those, or perhaps make them
dependent on the 'assume' value.

The key thing is not to solve the problem at the right level, rather
than adding lots of checks to avoid a mine.

Regards,
Simon