[2/4] of: property: Do not link to disabled devices
diff mbox series

Message ID 20200415150550.28156-3-nsaenzjulienne@suse.de
State Superseded
Headers show
Series
  • of: property: fw_devlink misc fixes
Related show

Commit Message

Nicolas Saenz Julienne April 15, 2020, 3:05 p.m. UTC
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@suse.de>
---
 drivers/of/property.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Saravana Kannan April 15, 2020, 6:30 p.m. UTC | #1
On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> 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@suse.de>
> ---
>  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.

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.

-Saravana
Nicolas Saenz Julienne April 16, 2020, 11:37 a.m. UTC | #2
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@suse.de> 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@suse.de>
> > ---
> >  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.

Regards,
Nicolas
Saravana Kannan April 16, 2020, 8:56 p.m. UTC | #3
On Thu, Apr 16, 2020 at 4:37 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> 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@suse.de> 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@suse.de>
> > > ---
> > >  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

Patch
diff mbox series

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;
+	}
+
 	/*
 	 * Don't allow linking a device node as a consumer of one of its
 	 * descendant nodes. By definition, a child node can't be a functional