[PATCH v2 0/2] of: property: fw_devlink misc fixes

[PATCH v2 0/2] of: property: fw_devlink misc fixes

[Nicolas Saenz Julienne]

As I'm interested in using this feature to fine-tune Raspberry Pi 4's
device probe dependencies, I tried to get the board to boot with
fw_devlink=on. As of today's linux-next the board won't boot with that
option. I tried to address the underlying issues.

---

Changes since v1:
 - Address Saravana's comments
 - Drop patch #3 & #4

Nicolas Saenz Julienne (2):
  of: property: Fix create device links for all child-supplier
    dependencies
  of: property: Do not link to disabled devices

 drivers/of/property.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

-- 
2.26.0

[PATCH v2 1/2] of: property: Fix create device links for all child-supplier dependencies

[Nicolas Saenz Julienne]

Upon adding a new device from a DT node, we scan its properties and its
children's properties in order to create a consumer/supplier
relationship between the device and the property provider.

That said, it's possible for some of the node's children to be disabled,
which will create links that'll never be fulfilled.

To get around this, use the for_each_available_child_of_node() function
instead of for_each_available_node() when iterating over the node's
children.

Fixes: d4387cd11741 ("of: property: Create device links for all child-supplier depencencies")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

---

Changes since v1:
 - Slightly reword description

 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 252e4f6001553..dc034eb45defd 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1298,7 +1298,7 @@ static int of_link_to_suppliers(struct device *dev,
 		if (of_link_property(dev, con_np, p->name))
 			ret = -ENODEV;
 
-	for_each_child_of_node(con_np, child)
+	for_each_available_child_of_node(con_np, child)
 		if (of_link_to_suppliers(dev, child) && !ret)
 			ret = -EAGAIN;
 
-- 
2.26.0

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

[Nicolas Saenz Julienne]

When creating a consumer/supplier relationship between two devices,
make sure the supplier node is actually active. Otherwise this will
create a link relationship 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>

---

Changes since v1:
 - Move availability check into the compatible search routine and bail
   if device node disabled

 drivers/of/property.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index dc034eb45defd..14b6266dd054b 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1045,8 +1045,25 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 	 * Find the device node that contains the supplier phandle.  It may be
 	 * @sup_np or it may be an ancestor of @sup_np.
 	 */
-	while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+	while (sup_np) {
+
+		/*
+		 * 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;
+		}
+
+		if (of_find_property(sup_np, "compatible", NULL))
+			break;
+
 		sup_np = of_get_next_parent(sup_np);
+	}
+
 	if (!sup_np) {
 		dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
 		return -ENODEV;
-- 
2.26.0

Re: [PATCH v2 0/2] of: property: fw_devlink misc fixes

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Hi Saravana,

On Fri, 2020-04-17 at 18:54 +0200, Nicolas Saenz Julienne wrote:
> As I'm interested in using this feature to fine-tune Raspberry Pi 4's
> device probe dependencies, I tried to get the board to boot with
> fw_devlink=on. As of today's linux-next the board won't boot with that
> option. I tried to address the underlying issues.
> 

On a semi-related topic, have you looked at vendor specific properties? most of
them create a consumer/supplier relationship, it'd be nice to be able to take
those ones into account as well.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] of: property: fw_devlink misc fixes

[Saravana Kannan]

On Fri, Apr 17, 2020 at 11:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Saravana,
>
> On Fri, 2020-04-17 at 18:54 +0200, Nicolas Saenz Julienne wrote:
> > As I'm interested in using this feature to fine-tune Raspberry Pi 4's
> > device probe dependencies, I tried to get the board to boot with
> > fw_devlink=on. As of today's linux-next the board won't boot with that
> > option. I tried to address the underlying issues.
> >
>
> On a semi-related topic, have you looked at vendor specific properties? most of
> them create a consumer/supplier relationship, it'd be nice to be able to take
> those ones into account as well.

I'm on the wall about that. If we take every vendor specific property,
this file will explode. Not sure I want to do that.

Also, we haven't even finished all the generic bindings. I'm just
adding bindings as I get familiar with each of them and I test them on
hardware I have lying around before sending it out. So, there's where
my focus is right now wrt fw_devlink and DT.

I wonder how many of the vendor specific properties do very similar
things and got in over time. Maybe they can be made generic? What one
did you have in mind?

-Saravana

Re: [PATCH v2 1/2] of: property: Fix create device links for all child-supplier dependencies

[Saravana Kannan]

On Fri, Apr 17, 2020 at 9:54 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Upon adding a new device from a DT node, we scan its properties and its
> children's properties in order to create a consumer/supplier
> relationship between the device and the property provider.
>
> That said, it's possible for some of the node's children to be disabled,
> which will create links that'll never be fulfilled.
>
> To get around this, use the for_each_available_child_of_node() function
> instead of for_each_available_node() when iterating over the node's
> children.
>
> Fixes: d4387cd11741 ("of: property: Create device links for all child-supplier depencencies")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>
> ---
>
> Changes since v1:
>  - Slightly reword description
>
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 252e4f6001553..dc034eb45defd 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1298,7 +1298,7 @@ static int of_link_to_suppliers(struct device *dev,
>                 if (of_link_property(dev, con_np, p->name))
>                         ret = -ENODEV;
>
> -       for_each_child_of_node(con_np, child)
> +       for_each_available_child_of_node(con_np, child)
>                 if (of_link_to_suppliers(dev, child) && !ret)
>                         ret = -EAGAIN;
>

Reviewed-by: Saravana Kannan <saravanak@google.com>

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

[Saravana Kannan]

On Fri, Apr 17, 2020 at 9:54 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 link relationship 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>
>
> ---
>
> Changes since v1:
>  - Move availability check into the compatible search routine and bail
>    if device node disabled
>
>  drivers/of/property.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index dc034eb45defd..14b6266dd054b 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1045,8 +1045,25 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>          * Find the device node that contains the supplier phandle.  It may be
>          * @sup_np or it may be an ancestor of @sup_np.
>          */
> -       while (sup_np && !of_find_property(sup_np, "compatible", NULL))
> +       while (sup_np) {
> +
> +               /*
> +                * Don't allow linking a device node as consumer of a disabled
> +                * node.
> +                */

Minor nit: I'd just say "Don't allow linking to a disabled supplier".

> +               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;
> +               }

This if block looks very similar to the one right after the loop.
Maybe there's a nice way to combine it?

If you replace this if block with this, it'll end up with the same result.
if (!of_device_is_available(sup_np)) {
        of_node_put(sup_np);
        sup_np = NULL;
}

of_get_next_parent() handles a NULL input properly. So that won't be a
problem. And "No device" is a valid statement for both cases I think.

> +
> +               if (of_find_property(sup_np, "compatible", NULL))
> +                       break;
> +
>                 sup_np = of_get_next_parent(sup_np);
> +       }
> +
>         if (!sup_np) {
>                 dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
>                 return -ENODEV;

However, not against this patch as is if Rob/Frank like it as is.

-Saravana

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

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]

On Fri, 2020-04-17 at 14:08 -0700, Saravana Kannan wrote:
> On Fri, Apr 17, 2020 at 9:54 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 link relationship 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>
> > 
> > ---
> > 
> > Changes since v1:
> >  - Move availability check into the compatible search routine and bail
> >    if device node disabled
> > 
> >  drivers/of/property.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index dc034eb45defd..14b6266dd054b 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1045,8 +1045,25 @@ static int of_link_to_phandle(struct device *dev,
> > struct device_node *sup_np,
> >          * Find the device node that contains the supplier phandle.  It may
> > be
> >          * @sup_np or it may be an ancestor of @sup_np.
> >          */
> > -       while (sup_np && !of_find_property(sup_np, "compatible", NULL))
> > +       while (sup_np) {
> > +
> > +               /*
> > +                * Don't allow linking a device node as consumer of a
> > disabled
> > +                * node.
> > +                */
> 
> Minor nit: I'd just say "Don't allow linking to a disabled supplier".
> 
> > +               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;
> > +               }
> 
> This if block looks very similar to the one right after the loop.
> Maybe there's a nice way to combine it?
> 
> If you replace this if block with this, it'll end up with the same result.
> if (!of_device_is_available(sup_np)) {
>         of_node_put(sup_np);
>         sup_np = NULL;
> }
> 
> of_get_next_parent() handles a NULL input properly. So that won't be a
> problem. And "No device" is a valid statement for both cases I think.
> 
> > +
> > +               if (of_find_property(sup_np, "compatible", NULL))
> > +                       break;
> > +
> >                 sup_np = of_get_next_parent(sup_np);
> > +       }
> > +
> >         if (!sup_np) {
> >                 dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
> >                 return -ENODEV;
> 
> However, not against this patch as is if Rob/Frank like it as is.

Agree with your suggestions, I'll send an v3.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] of: property: fw_devlink misc fixes

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]

Hi Saravana,

On Fri, 2020-04-17 at 13:55 -0700, Saravana Kannan wrote:
> On Fri, Apr 17, 2020 at 11:06 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Saravana,
> > 
> > On Fri, 2020-04-17 at 18:54 +0200, Nicolas Saenz Julienne wrote:
> > > As I'm interested in using this feature to fine-tune Raspberry Pi 4's
> > > device probe dependencies, I tried to get the board to boot with
> > > fw_devlink=on. As of today's linux-next the board won't boot with that
> > > option. I tried to address the underlying issues.
> > > 
> > 
> > On a semi-related topic, have you looked at vendor specific properties? most
> > of
> > them create a consumer/supplier relationship, it'd be nice to be able to
> > take
> > those ones into account as well.
> 
> I'm on the wall about that. If we take every vendor specific property,
> this file will explode. Not sure I want to do that.
> 
> Also, we haven't even finished all the generic bindings. I'm just
> adding bindings as I get familiar with each of them and I test them on
> hardware I have lying around before sending it out. So, there's where
> my focus is right now wrt fw_devlink and DT.
> 
> I wonder how many of the vendor specific properties do very similar
> things and got in over time. Maybe they can be made generic? What one
> did you have in mind?

Well, long story short, we need to create a relationship between RPi4's PCI bus
(which hangs from an interconnect in DT) and RPi4's co-processor, which has a
highly unconventional firmware driver (raspberrypi.c in drivers/firmware). The
PCI bus just needs the co-processor interface to be up before probing, that's
all (I'll spare you the details of why). Ideally we want to avoid adding
platform specific code into an otherwise generic bus driver as it'll be used by
a number of unrelated SoCs, and it's generally frowned upon.

There is no generic property to handle this case, and it's very unlikely there
will ever be one, since these firmware drivers have very little in common. I
guess this could make an argument for a generic _last resort only_
'supplied-by' property, but I bet this solution won't be very popular.

Another idea that comes to mind for vendor specific properties would be
exporting a macro in the lines of "DEFINE_SIMPLE_PROP()" for supplier drivers
to define custom properties. The parse_prop() callbacks could then be added
into a special section for of/property.c to pickup and parse. The good thing is
that the list length would be limited by the kernel configuration and the
maintenance burden moved to the driver authors, at least to some extent.

Anyway, if something comes to mind to solve RPi4's situation feel free to
propose anything :).

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] of: property: fw_devlink misc fixes

[Saravana Kannan]

On Mon, Apr 20, 2020 at 4:29 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Saravana,
>
> On Fri, 2020-04-17 at 13:55 -0700, Saravana Kannan wrote:
> > On Fri, Apr 17, 2020 at 11:06 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > > Hi Saravana,
> > >
> > > On Fri, 2020-04-17 at 18:54 +0200, Nicolas Saenz Julienne wrote:
> > > > As I'm interested in using this feature to fine-tune Raspberry Pi 4's
> > > > device probe dependencies, I tried to get the board to boot with
> > > > fw_devlink=on. As of today's linux-next the board won't boot with that
> > > > option. I tried to address the underlying issues.
> > > >
> > >
> > > On a semi-related topic, have you looked at vendor specific properties? most
> > > of
> > > them create a consumer/supplier relationship, it'd be nice to be able to
> > > take
> > > those ones into account as well.
> >
> > I'm on the wall about that. If we take every vendor specific property,
> > this file will explode. Not sure I want to do that.
> >
> > Also, we haven't even finished all the generic bindings. I'm just
> > adding bindings as I get familiar with each of them and I test them on
> > hardware I have lying around before sending it out. So, there's where
> > my focus is right now wrt fw_devlink and DT.
> >
> > I wonder how many of the vendor specific properties do very similar
> > things and got in over time. Maybe they can be made generic? What one
> > did you have in mind?
>
> Well, long story short, we need to create a relationship between RPi4's PCI bus
> (which hangs from an interconnect in DT) and RPi4's co-processor, which has a
> highly unconventional firmware driver (raspberrypi.c in drivers/firmware). The
> PCI bus just needs the co-processor interface to be up before probing,

I'm guessing it still works fine today by doing a deferred probe and
you are just trying to avoid having to do a deferred probe? I haven't
kept track of RPi4's upstream support status.

> that's
> all (I'll spare you the details of why). Ideally we want to avoid adding
> platform specific code into an otherwise generic bus driver as it'll be used by
> a number of unrelated SoCs, and it's generally frowned upon.

Which PCI driver is that specifically (I'm sure I can dig around to
find RPi4's DT and figure it out, but it's easier to just ask :) ) ?
Also, can you point me to the DT and the nodes that we are talking
about here (the PCI and the firmware nodes)?

> There is no generic property to handle this case, and it's very unlikely there
> will ever be one, since these firmware drivers have very little in common. I
> guess this could make an argument for a generic _last resort only_
> 'supplied-by' property, but I bet this solution won't be very popular.

Ha, this was my initial idea for the whole fw_devlink feature. I
called it depends-on. Rob/Frank convinced me to instead just parse the
existing bindings -- which was definitely the right call. Otherwise DT
would have been a mess. Adding support for "depends-on" for one off
use cases might still be a touchy topic. I myself am on the wall. It's
useful for some rare cases, but it's also very easy to abuse.

> Another idea that comes to mind for vendor specific properties would be
> exporting a macro in the lines of "DEFINE_SIMPLE_PROP()" for supplier drivers
> to define custom properties. The parse_prop() callbacks could then be added
> into a special section for of/property.c to pickup and parse. The good thing is
> that the list length would be limited by the kernel configuration and the
> maintenance burden moved to the driver authors, at least to some extent.

I did think about this option too sometime back, but not too keen on
that -- at least at this point. Mostly because there are other issues
I want to resolve first and want to get their design right before
getting to hand off property parsing to other drivers/files.

> Anyway, if something comes to mind to solve RPi4's situation feel free to
> propose anything :).

I'll let you know if I do. I'm not sure I fully understand the issue
though. I don't think you are trying to avoid deferred probes because
you aren't trying to improve boot time. I'm guessing the PCI bus
driver in question has no way to know when to defer the probe, in this
case you are trying to fix?

-Saravana

Re: [PATCH v2 0/2] of: property: fw_devlink misc fixes

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

On Mon, 2020-04-20 at 15:37 -0700, Saravana Kannan wrote:

[...]

> On Mon, Apr 20, 2020 at 4:29 AM Nicolas Saenz Julienne
> > Well, long story short, we need to create a relationship between RPi4's PCI
> > bus
> > (which hangs from an interconnect in DT) and RPi4's co-processor, which has
> > a
> > highly unconventional firmware driver (raspberrypi.c in drivers/firmware).
> > The
> > PCI bus just needs the co-processor interface to be up before probing,
> 
> I'm guessing it still works fine today by doing a deferred probe and
> you are just trying to avoid having to do a deferred probe? I haven't
> kept track of RPi4's upstream support status.

Yes that's the idea, and here's the patch I'm trying to avoid:
https://lkml.org/lkml/2020/3/24/1508

> > that's
> > all (I'll spare you the details of why). Ideally we want to avoid adding
> > platform specific code into an otherwise generic bus driver as it'll be used
> > by
> > a number of unrelated SoCs, and it's generally frowned upon.
> 
> Which PCI driver is that specifically (I'm sure I can dig around to
> find RPi4's DT and figure it out, but it's easier to just ask :) ) ?
> Also, can you point me to the DT and the nodes that we are talking
> about here (the PCI and the firmware nodes)?

So the PCI driver is pcie-brcmstb.c, its DT node is available in bcm2711.dtsi
(search for pcie0), the firmware interface is defined in bcm2835-rpi.dtsi
(search for firmware).

> > There is no generic property to handle this case, and it's very unlikely
> > there
> > will ever be one, since these firmware drivers have very little in common. I
> > guess this could make an argument for a generic _last resort only_
> > 'supplied-by' property, but I bet this solution won't be very popular.
> 
> Ha, this was my initial idea for the whole fw_devlink feature. I
> called it depends-on. Rob/Frank convinced me to instead just parse the
> existing bindings -- which was definitely the right call. Otherwise DT
> would have been a mess. Adding support for "depends-on" for one off
> use cases might still be a touchy topic. I myself am on the wall. It's
> useful for some rare cases, but it's also very easy to abuse.

Yes, I agree.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]