Hi Tom, On Fri, 21 Nov 2025 at 07:35, Tom Rini <[email protected]> wrote: > > On Fri, Nov 21, 2025 at 07:25:46AM -0700, Simon Glass wrote: > > 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 Wed, 19 Nov 2025 at 21:14, Marek Vasut > > > > <[email protected]> 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. > > > >> > > > >> Add dev_has_ofnode(dev) check and do not assign clock 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 <[email protected]> > > > >> --- > > > >> Cc: Patrice Chotard <[email protected]> > > > >> Cc: Patrick Delaunay <[email protected]> > > > >> Cc: Sean Anderson <[email protected]> > > > >> Cc: Tom Rini <[email protected]> > > > >> Cc: [email protected] > > > >> --- > > > >> drivers/clk/clk_fixed_rate.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/clk/clk_fixed_rate.c > > > >> b/drivers/clk/clk_fixed_rate.c > > > >> index 95a77d2e041..5c32ae207fe 100644 > > > >> --- a/drivers/clk/clk_fixed_rate.c > > > >> +++ b/drivers/clk/clk_fixed_rate.c > > > >> @@ -35,7 +35,7 @@ void clk_fixed_rate_ofdata_to_plat_(struct udevice > > > >> *dev, > > > >> struct clk_fixed_rate *plat) > > > >> { > > > >> struct clk *clk = &plat->clk; > > > >> - if (CONFIG_IS_ENABLED(OF_REAL)) > > > >> + if (CONFIG_IS_ENABLED(OF_REAL) && dev_has_ofnode(dev)) > > > >> plat->fixed_rate = dev_read_u32_default(dev, > > > >> "clock-frequency", > > > >> 0); > > > > > > > > What is happening here? It should already be checking this: > > > > > > > > static inline __attribute_const__ bool dev_has_ofnode(const struct > > > > udevice *dev) > > > > { > > > > #if CONFIG_IS_ENABLED(OF_REAL) > > > > return ofnode_valid(dev_ofnode(dev)); > > > > #else > > > > return false; > > > > #endif > > > > } > > > I do not understand the question -- this patch _adds_ the > > > dev_has_ofnode() test into clk_fixed_rate_ofdata_to_plat_() , it wasn't > > > present before. Can you please rephrase your question / concern ? > > > > The dev_has_ofnode() function already checks OF_REAL, as shown above. To > > repeat: > > > > > > static inline __attribute_const__ bool dev_has_ofnode(const struct > > > > udevice *dev) > > > > { > > > > #if CONFIG_IS_ENABLED(OF_REAL) > > > > return ofnode_valid(dev_ofnode(dev)); > > > > #else > > > > return false; > > > > ^^ returns false here > > > > > > #endif > > > > You are adding another check in the caller: > > > > > >> - if (CONFIG_IS_ENABLED(OF_REAL)) > > > >> + if (CONFIG_IS_ENABLED(OF_REAL) && dev_has_ofnode(dev)) > > > > dev_has_ofnode() will already return false if OF_REAL is not enabled. > > So what is the point of checking OF_REAL before calling > > dev_has_ofnode() ? > > > > I don't understand what benefit this patch brings. > > I think the misunderstanding is that you're asking for this: > - if (CONFIG_IS_ENABLED(OF_REAL)) > + if (dev_has_ofnode(dev)) > > To be made to clk_fixed_rate_ofdata_to_plat_(...) in > drivers/clk/clk_fixed_rate.c as it does not currently call > dev_has_ofnode(dev).
Ah OK. So the call stack is something like: dev_read_u32_default() dev_ofnode() - returns ofnode_null ofnode_read_u32_default() ofnode_read_u32_index fdt_getprop(NULL, -1, ...) // guess And I see that I put assert() in some of the ofnode_read..() functions because I don't have tests for passing an invalid ofnode. So perhaps fdt_offset_ptr_() should check for NULL and return? I haven't traced it though. Regards, Simon
