[PATCH] clk: fixed_rate: Avoid calling dev_read_*() if CONFIG_OF_PLATDATA=y

[Simon Glass]

Hi Tom,

On Fri, 21 Nov 2025 at 07:35, Tom Rini <trini at konsulko.com> 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 <marek.vasut at mailbox.org> wrote:
> > >
> > > On 11/21/25 12:40 AM, Simon Glass wrote:
> > > > Hi Marek,
> > > >
> > > > On Wed, 19 Nov 2025 at 21:14, Marek Vasut
> > > > <marek.vasut+renesas at mailbox.org> 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 <marek.vasut+renesas at mailbox.org>
> > > >> ---
> > > >> Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> > > >> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > >> Cc: Sean Anderson <seanga2 at gmail.com>
> > > >> Cc: Tom Rini <trini at konsulko.com>
> > > >> Cc: u-boot at lists.denx.de
> > > >> ---
> > > >>   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