Hi Marek, On Thu, 20 Nov 2025 at 21:39, Marek Vasut <[email protected]> wrote: > > On 11/21/25 12:40 AM, Simon Glass wrote: > > Hi Marek, > > > > On Thu, 20 Nov 2025 at 04:56, Marek Vasut <[email protected]> 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
