[PATCH v2 1/8] event: signal when livetree has been built

[Simon Glass]

Hi Caleb,

On Mon, 14 Apr 2025 at 06:33, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Simon,
>
> On 4/11/25 20:27, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Fri, 11 Apr 2025 at 06:47, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>
> >> OF_LIVE offers a variety of benefits, one of them being that the live
> >> tree can be modified without caring about the underlying FDT. This is
> >> particularly valuable for working around U-Boot limitations like lacking
> >> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> >> peripherals like the sdcard being broken (and displaying potentially
> >> worrying error messages).
> >>
> >> Add an event to signal when the live tree has been built so that we can
> >> apply fixups to it directly before devices are bound.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >>   common/event.c  |  3 +++
> >>   include/event.h | 18 ++++++++++++++++++
> >>   lib/of_live.c   | 11 +++++++++++
> >>   3 files changed, 32 insertions(+)
> >>
> >> diff --git a/common/event.c b/common/event.c
> >> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> >> --- a/common/event.c
> >> +++ b/common/event.c
> >> @@ -47,8 +47,11 @@ const char *const type_name[] = {
> >>          "ft_fixup",
> >>
> >>          /* main loop events */
> >>          "main_loop",
> >> +
> >> +       /* livetree has been built */
> >> +       "of_live_init",
> >>   };
> >>
> >>   _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
> >>   #endif
> >> diff --git a/include/event.h b/include/event.h
> >> index 75141a192a48b0931667632f41be8ff4d6139f7c..1d267f1d10547642d381fa287ab4981a2bf03543 100644
> >> --- a/include/event.h
> >> +++ b/include/event.h
> >> @@ -152,8 +152,17 @@ enum event_t {
> >>           * A non-zero return value causes the boot to fail.
> >>           */
> >>          EVT_MAIN_LOOP,
> >>
> >> +       /**
> >> +        * @EVT_OF_LIVE_BUILT:
> >> +        * This event is triggered immediately after the live device tree has been
> >> +        * built. This allows for machine specific fixups to be done to the live tree
> >> +        * (like disabling known-unsupported devices) before it is used. This
> >> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
> >
> > Add a bit more detail, e.g.: before it is used by U-Boot
> > post-relocation. This means it is possible to affect the devices bound
> > by driver model.
>
> I think this is really implied and made obvious by context clues. IF
> there comes a time where we build the live tree more than once (e.g. to
> build it as part of the DT_FIXUP_PROTOCOL for some reason) then the
> extra info you propose adding would no longer be correct, where the
> current wording would remain correct.

OK, leave it alone if you like.

>
> >
> > You should also note that the flattree is inactive when livetree is
> > used and any changes may eventually be passed onto the OS, if it
> > doesn't have its own devicetree. This is the point we were discussing
> > on the other thread.
>
> This information is totally irrelevant though, and there is no broad
> agreement with your proposed changes. The livetree API is also set up so
> that one could build multiple livetrees (for whatever reason), this
> event is modelled to reflect that (where you can see in my v1 patches
> that the event assumed you would only ever build the global tree).

Hmmm, if Linaro is going to block livetree fixups then we are going to
have a problem. I have not heard that, but let me know.

>
> >
> >> +        */
> >> +       EVT_OF_LIVE_BUILT,
> >> +
> >>          /**
> >>           * @EVT_COUNT:
> >>           * This constants holds the maximum event number + 1 and is used when
> >>           * looping over all event classes.
> >> @@ -202,8 +211,17 @@ union event_data {
> >>          struct event_ft_fixup {
> >>                  oftree tree;
> >>                  struct bootm_headers *images;
> >>          } ft_fixup;
> >> +
> >> +       /**
> >> +        * struct event_of_live_built - livetree has been built
> >> +        *
> >> +        * @root: The root node of the live device tree
> >> +        */
> >> +       struct event_of_live_built {
> >> +               struct device_node *root;
> >> +       } of_live_built;
> >>   };
> >>
> >>   /**
> >>    * struct event - an event that can be sent and received
> >> diff --git a/lib/of_live.c b/lib/of_live.c
> >> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..c1620616513c2e32448b4a6d156a9162d97c76b7 100644
> >> --- a/lib/of_live.c
> >> +++ b/lib/of_live.c
> >> @@ -10,8 +10,9 @@
> >>
> >>   #define LOG_CATEGORY   LOGC_DT
> >>
> >>   #include <abuf.h>
> >> +#include <event.h>
> >>   #include <log.h>
> >>   #include <linux/libfdt.h>
> >>   #include <of_live.h>
> >>   #include <malloc.h>
> >> @@ -320,8 +321,9 @@ int unflatten_device_tree(const void *blob, struct device_node **mynodes)
> >>
> >>   int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >>   {
> >>          int ret;
> >> +       union event_data evt;
> >>
> >>          debug("%s: start\n", __func__);
> >>          ret = unflatten_device_tree(fdt_blob, rootp);
> >>          if (ret) {
> >> @@ -334,8 +336,17 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >>                  return ret;
> >>          }
> >>          debug("%s: stop\n", __func__);
> >>
> >> +       if (CONFIG_IS_ENABLED(EVENT)) {
> >> +               evt.of_live_built.root = *rootp;
> >> +               ret = event_notify(EVT_OF_LIVE_BUILT, &evt, sizeof(evt));
> >> +               if (ret) {
> >> +                       log_debug("Failed to notify livetree build event: err=%d\n", ret);
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +
> >
> > This should move to the caller of this function. We should also have a
>
> Why move it to the caller? Then every caller would need to signal the
> event and we'd pointlessly duplicate code. The event signals that /a/
> livetree has been built. Today this only happens once and we make the
> assumption that this livetree is to be used by U-Boot and only U-Boot.
>
> Should this assumption become incorrect in the future, it would be
> trivial to introduce a variable to the event_data that describes what
> the livetree is and/or where it will be used so that anyone catching the
> event can decide what to do.
>
> Since we don't yet know any other usecases, it would be overzealous to
> try and capture this information since we just don't know what's important.

There is (and will only ever be) one caller. At this point I really
don't mind what you do.

>
> > sandbox test for this feature, e.g. catch the event and add a
> > node/prop and check in a test in test-fdt.c (or similar) that it is
> > present.
>
> What would this test other than that the event is sent? We already have
> tests for the event framework...

If the CI tests pass, then it's fine.


>
> >
> > I'm not sure if you need to update test_event_dump.py but since CI
> > presumably passes, I guess not.>
> >>          return ret;
> >>   }
> >>
> >>   void of_live_free(struct device_node *root)
> >>
> >> --
> >> 2.49.0
> >>

Regards,
Simon