Re: [PATCH v1] of: platform: Batch fwnode parsing in the init_machine() path

[Rob Herring]

On Fri, Oct 2, 2020 at 12:52 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Fri, Oct 2, 2020 at 7:08 AM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> >
> > On Thu, Oct 1, 2020 at 5:59 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > >
> > > When commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when
> > > adding all top level devices") optimized the fwnode parsing when all top
> > > level devices are added, it missed out optimizing this for platform
> > > where the top level devices are added through the init_machine() path.
> > >
> > > This commit does the optimization for all paths by simply moving the
> > > fw_devlink_pause/resume() inside of_platform_default_populate().
> > >
> > > Reported-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> > > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > > ---
> > >  drivers/of/platform.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 071f04da32c8..79972e49b539 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -501,8 +501,21 @@ int of_platform_default_populate(struct device_node *root,
> > >                                  const struct of_dev_auxdata *lookup,
> > >                                  struct device *parent)
> > >  {
> > > -       return of_platform_populate(root, of_default_bus_match_table, lookup,
> > > -                                   parent);
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * fw_devlink_pause/resume() are only safe to be called around top
> > > +        * level device addition due to locking constraints.
> > > +        */
> > > +       if (!root)
> > > +               fw_devlink_pause();
> > > +
> > > +       ret = of_platform_populate(root, of_default_bus_match_table, lookup,
> > > +                                  parent);
> >
> > of_platform_default_populate() vs. of_platform_populate() is just a
> > different match table. I don't think the behavior should otherwise be
> > different.
> >
> > There's also of_platform_probe() which has slightly different matching
> > behavior. It should not behave differently either with respect to
> > devlinks.
>
> So I'm trying to do this only when the top level devices are added for
> the first time. of_platform_default_populate() seems to be the most
> common path. For other cases, I think we just need to call
> fw_devlink_pause/resume() wherever the top level devices are added for
> the first time.

> As I said in the other email, we can't add
> fw_devlink_pause/resume() by default to of_platform_populate().

If you detect it's the first time, you could?

>
> Do you have other ideas for achieving "call fw_devlink_pause/resume()
> only when top level devices are added for the first time"?

Eliminate the cases not using of_platform_default_populate().

There's 2 main reasons for the non default cases. The first is
auxdata. Really, for any modern platform that people care about (and
care about the boot time), they should not be using auxdata. That's
just for the DT transition. You know, a temporary thing from 9 years
ago.

The 2nd is having some parent device. This is typically an soc_device.
I really think this is kind of dumb. We should either have the parent
device always or never. After all, everything's an SoC right? Of
course changing that will break some Android systems since they like
to use non-ABI sysfs device paths.

There could also be some initcall ordering issues. IIRC, in the last
round of cleanups in this area, at91 gpio/pinctrl had an issue with
that. I think I have a half done fix for that I started.

Rob