Re: [PATCH 2/4] of: property: Do not link to disabled devices

[Saravana Kannan]

On Thu, Apr 16, 2020 at 4:37 AM Nicolas Saenz Julienne
<nsaenzjulienne@xxxxxxx> wrote:
>
> On Wed, 2020-04-15 at 11:30 -0700, Saravana Kannan wrote:
> > On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@xxxxxxx> wrote:
> > > When creating a consumer/supplier relationship between two devices, make
> > > sure the supplier node is actually active. Otherwise this will create a
> > > device link that will never be fulfilled. This, in the worst case
> > > scenario, will hang the system during boot.
> > >
> > > Note that, in practice, the fact that a device-tree represented
> > > consumer/supplier relationship isn't fulfilled will not prevent devices
> > > from successfully probing.
> > >
> > > Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT
> > > bindings")
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
> > > ---
> > >  drivers/of/property.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index a8c2b13521b27..487685ff8bb19 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1052,6 +1052,13 @@ static int of_link_to_phandle(struct device *dev,
> > > struct device_node *sup_np,
> > >                 return -ENODEV;
> > >         }
> > >
> > > +       /* Don't allow linking a device node as consumer of a disabled node
> > > */
> > > +       if (!of_device_is_available(sup_np)) {
> > > +               dev_dbg(dev, "Not linking to %pOFP - Not available\n",
> > > sup_np);
> > > +               of_node_put(sup_np);
> > > +               return -ENODEV;
> > > +       }
> > > +
> >
> > Again, surprised I haven't hit this situation with the number of
> > disabled devices I have.
>
> I'll point out to the example that triggered this issue on my reply to patch
> #4.
>
> > The idea is right, but the implementation can be better. I think this
> > check needs to be the first check after the of_node_get(sup_np) --
> > before we do any of the "walk up to find the device" part.
> >
> > Otherwise, you could have a supplier device (the one with compatible
> > prop) that's available with a child node that's disabled. And the
> > phandle could be pointing to that disabled child node. If you don't do
> > this as the first check, you might still try to form a pointless
> > device link. It won't affect probing (because the actual struct device
> > will probe) but it's still a pointless device link and a pointless
> > delay in probing, etc.
>
> Agree, I'll update the patch.

I thought about it more. I think you should do this check in the loop
that's walking up to the "compatible" node because any node in that
path having status=disabled would/should disable this supplier if I
understand DT correctly. Technically we need to do this all the way up
to the root, but we'll do that if we have actual reports of that
causing problems. Otherwise, it's just wasteful.

-Saravana