[PATCH v1 0/2] Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

[PATCH v1 0/2] Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

[Saravana Kannan]

Ulf, mind testing this?

Thanks,
Saravana
[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Cc: Ulf Hansson <ulf.hansson@linaro.org>

Saravana Kannan (2):
  driver core: Add support for FWNODE_FLAG_NEVER_PROBES
  of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

 drivers/base/core.c    |  8 ++++++++
 drivers/of/platform.c  | 16 ++++++++++++++++
 include/linux/fwnode.h |  8 +++++---
 3 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.33.0.259.gc128427fd7-goog

[PATCH v1 1/2] driver core: Add support for FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

Add a fwnode flag to mark a device as one that will never be probed even
though it's added to a bus.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 8 ++++++++
 include/linux/fwnode.h | 8 +++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index f6360490a4a3..5d14897a53e6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1719,6 +1719,14 @@ static int fw_devlink_create_devlink(struct device *con,
 	struct device *sup_dev;
 	int ret = 0;
 
+	/*
+	 * If the consumer or supplier is a device that'll never be probed,
+	 * don't create devices link for it.
+	 */
+	if (con->fwnode->flags & FWNODE_FLAG_NEVER_PROBES ||
+	    sup_handle->flags & FWNODE_FLAG_NEVER_PROBES)
+		return -EINVAL;
+
 	sup_dev = get_dev_from_fwnode(sup_handle);
 	if (sup_dev) {
 		/*
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 59828516ebaf..3ad667ae97d8 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -19,13 +19,15 @@ struct device;
 /*
  * fwnode link flags
  *
- * LINKS_ADDED:	The fwnode has already be parsed to add fwnode links.
- * NOT_DEVICE:	The fwnode will never be populated as a struct device.
- * INITIALIZED: The hardware corresponding to fwnode has been initialized.
+ * LINKS_ADDED:	 The fwnode has already be parsed to add fwnode links.
+ * NOT_DEVICE:	 The fwnode will never be populated as a struct device.
+ * INITIALIZED:  The hardware corresponding to fwnode has been initialized.
+ * NEVER_PROBES: The device that corresponds to this fwnode will never probe.
  */
 #define FWNODE_FLAG_LINKS_ADDED		BIT(0)
 #define FWNODE_FLAG_NOT_DEVICE		BIT(1)
 #define FWNODE_FLAG_INITIALIZED		BIT(2)
+#define FWNODE_FLAG_NEVER_PROBES	BIT(3)
 
 struct fwnode_handle {
 	struct fwnode_handle *secondary;
-- 
2.33.0.259.gc128427fd7-goog

[PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

We don't want fw_devlink creating device links for bus devices as
they'll never probe. So mark those device node with this flag.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 74afbb7a4f5e..42b3936d204a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
 	if (!dev || !of_match_node(matches, bus))
 		return 0;
 
+	/*
+	 * If the bus node has only one compatible string value and it has
+	 * matched as a bus node, it's never going to get probed by a device
+	 * driver. So flag it as such so that fw_devlink knows not to create
+	 * device links with this device.
+	 *
+	 * This doesn't catch all devices that'll never probe, but this is good
+	 * enough for now.
+	 *
+	 * This doesn't really work for PPC because of how it uses
+	 * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
+	 */
+	if (!IS_ENABLED(CONFIG_PPC) &&
+	    of_property_count_strings(bus, "compatible") == 1)
+		bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
+
 	for_each_child_of_node(bus, child) {
 		pr_debug("   create child: %pOF\n", child);
 		rc = of_platform_bus_create(child, matches, lookup, &dev->dev, strict);
-- 
2.33.0.259.gc128427fd7-goog

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Rob Herring]

On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
>
> We don't want fw_devlink creating device links for bus devices as
> they'll never probe. So mark those device node with this flag.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/platform.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 74afbb7a4f5e..42b3936d204a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
>         if (!dev || !of_match_node(matches, bus))
>                 return 0;
>
> +       /*
> +        * If the bus node has only one compatible string value and it has
> +        * matched as a bus node, it's never going to get probed by a device
> +        * driver. So flag it as such so that fw_devlink knows not to create
> +        * device links with this device.
> +        *
> +        * This doesn't catch all devices that'll never probe, but this is good
> +        * enough for now.
> +        *
> +        * This doesn't really work for PPC because of how it uses
> +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> +        */
> +       if (!IS_ENABLED(CONFIG_PPC) &&
> +           of_property_count_strings(bus, "compatible") == 1)
> +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;

This looks fragile relying on 1 compatible string, and the DT flags in
this code have been fragile too. I'm pretty sure we have cases of
simple-bus or simple-mfd that also have another compatible.

Couldn't we solve this with a simple driver? Make 'simple-pm-bus'
driver work for other cases? BTW, this patch doesn't even work for
simple-pm-bus. A driver for simple-bus may cause issues if there's a
more specific driver to bind to as we don't handle that. It's simply
whichever matches first.

Rob

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > We don't want fw_devlink creating device links for bus devices as
> > they'll never probe. So mark those device node with this flag.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/of/platform.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 74afbb7a4f5e..42b3936d204a 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> >         if (!dev || !of_match_node(matches, bus))
> >                 return 0;
> >
> > +       /*
> > +        * If the bus node has only one compatible string value and it has
> > +        * matched as a bus node, it's never going to get probed by a device
> > +        * driver. So flag it as such so that fw_devlink knows not to create
> > +        * device links with this device.
> > +        *
> > +        * This doesn't catch all devices that'll never probe, but this is good
> > +        * enough for now.
> > +        *
> > +        * This doesn't really work for PPC because of how it uses
> > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > +        */
> > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > +           of_property_count_strings(bus, "compatible") == 1)
> > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
>
> This looks fragile relying on 1 compatible string, and the DT flags in
> this code have been fragile too. I'm pretty sure we have cases of
> simple-bus or simple-mfd that also have another compatible.
>
> Couldn't we solve this with a simple driver?

Oh, I didn't think you'd like that. I'd lean towards that option too
if we can address some of the other concerns below.

> Make 'simple-pm-bus'
> driver work for other cases?

> BTW, this patch doesn't even work for
> simple-pm-bus.

How do you mean? Because simple-pm-bus already has a driver and
doesn't set "matches" param when it calls of_platform_populate() and
this flag won't be set. So at least for simple-pm-bus I don't see any
issue.

I was trying to reuse of_default_bus_match_table without explicitly
referring to it, but if it's confusing I can add a separate list of
compatible strings and use those here instead of using "matches".

> A driver for simple-bus may cause issues if there's a
> more specific driver to bind to as we don't handle that. It's simply
> whichever matches first.

Right, this is my worry. Especially for devices like this (there are
plenty of cases like this) which have a driver that probes them but
also lists simple-bus
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299

So as long as there's a compatible string that's not one of the
"transparent" busses, this driver shouldn't match. So, I don't think I
can get away from checking the compatible strings.

How about I check here to make sure all the "compatible" strings are
from an approved transparent bus list, and if it's true, I use
driver_override to force match it to a transparent bus driver? Would
you be okay with that?

-Saravana

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Rob Herring]

On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > We don't want fw_devlink creating device links for bus devices as
> > > they'll never probe. So mark those device node with this flag.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/of/platform.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 74afbb7a4f5e..42b3936d204a 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > >         if (!dev || !of_match_node(matches, bus))
> > >                 return 0;
> > >
> > > +       /*
> > > +        * If the bus node has only one compatible string value and it has
> > > +        * matched as a bus node, it's never going to get probed by a device
> > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > +        * device links with this device.
> > > +        *
> > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > +        * enough for now.
> > > +        *
> > > +        * This doesn't really work for PPC because of how it uses
> > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > +        */
> > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > +           of_property_count_strings(bus, "compatible") == 1)
> > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> >
> > This looks fragile relying on 1 compatible string, and the DT flags in
> > this code have been fragile too. I'm pretty sure we have cases of
> > simple-bus or simple-mfd that also have another compatible.
> >
> > Couldn't we solve this with a simple driver?
>
> Oh, I didn't think you'd like that. I'd lean towards that option too
> if we can address some of the other concerns below.
>
> > Make 'simple-pm-bus'
> > driver work for other cases?
>
> > BTW, this patch doesn't even work for
> > simple-pm-bus.
>
> How do you mean? Because simple-pm-bus already has a driver and
> doesn't set "matches" param when it calls of_platform_populate() and
> this flag won't be set. So at least for simple-pm-bus I don't see any
> issue.

You're right.

> I was trying to reuse of_default_bus_match_table without explicitly
> referring to it, but if it's confusing I can add a separate list of
> compatible strings and use those here instead of using "matches".

What happens with a non-default table? I'm not sure we can assume the
same behavior.

> > A driver for simple-bus may cause issues if there's a
> > more specific driver to bind to as we don't handle that. It's simply
> > whichever matches first.
>
> Right, this is my worry. Especially for devices like this (there are
> plenty of cases like this) which have a driver that probes them but
> also lists simple-bus
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299

Uhh, that one is certainly a leakage of wanting an soc_device in the
hierarchy, not any real bus structure reflecting the h/w. I'm not a
fan of the soc_device stuff and its optional nature. Everything is an
SoC, so it should always be there? Or your device hierarchy should
change when you decide to add a soc_device?

> So as long as there's a compatible string that's not one of the
> "transparent" busses, this driver shouldn't match. So, I don't think I
> can get away from checking the compatible strings.
>
> How about I check here to make sure all the "compatible" strings are
> from an approved transparent bus list, and if it's true, I use
> driver_override to force match it to a transparent bus driver? Would
> you be okay with that?

Can't we do that within a driver? We check this and fail probe if
there's a more specific compatible.  Then another driver can match and
probe.

Rob

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > We don't want fw_devlink creating device links for bus devices as
> > > > they'll never probe. So mark those device node with this flag.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/of/platform.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > index 74afbb7a4f5e..42b3936d204a 100644
> > > > --- a/drivers/of/platform.c
> > > > +++ b/drivers/of/platform.c
> > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > > >         if (!dev || !of_match_node(matches, bus))
> > > >                 return 0;
> > > >
> > > > +       /*
> > > > +        * If the bus node has only one compatible string value and it has
> > > > +        * matched as a bus node, it's never going to get probed by a device
> > > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > > +        * device links with this device.
> > > > +        *
> > > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > > +        * enough for now.
> > > > +        *
> > > > +        * This doesn't really work for PPC because of how it uses
> > > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > > +        */
> > > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > > +           of_property_count_strings(bus, "compatible") == 1)
> > > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> > >
> > > This looks fragile relying on 1 compatible string, and the DT flags in
> > > this code have been fragile too. I'm pretty sure we have cases of
> > > simple-bus or simple-mfd that also have another compatible.
> > >
> > > Couldn't we solve this with a simple driver?
> >
> > Oh, I didn't think you'd like that. I'd lean towards that option too
> > if we can address some of the other concerns below.
> >
> > > Make 'simple-pm-bus'
> > > driver work for other cases?
> >
> > > BTW, this patch doesn't even work for
> > > simple-pm-bus.
> >
> > How do you mean? Because simple-pm-bus already has a driver and
> > doesn't set "matches" param when it calls of_platform_populate() and
> > this flag won't be set. So at least for simple-pm-bus I don't see any
> > issue.
>
> You're right.
>
> > I was trying to reuse of_default_bus_match_table without explicitly
> > referring to it, but if it's confusing I can add a separate list of
> > compatible strings and use those here instead of using "matches".
>
> What happens with a non-default table? I'm not sure we can assume the
> same behavior.
>
> > > A driver for simple-bus may cause issues if there's a
> > > more specific driver to bind to as we don't handle that. It's simply
> > > whichever matches first.
> >
> > Right, this is my worry. Especially for devices like this (there are
> > plenty of cases like this) which have a driver that probes them but
> > also lists simple-bus
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299
>
> Uhh, that one is certainly a leakage of wanting an soc_device in the
> hierarchy, not any real bus structure reflecting the h/w. I'm not a
> fan of the soc_device stuff and its optional nature. Everything is an
> SoC, so it should always be there? Or your device hierarchy should
> change when you decide to add a soc_device?
>
> > So as long as there's a compatible string that's not one of the
> > "transparent" busses, this driver shouldn't match. So, I don't think I
> > can get away from checking the compatible strings.
> >
> > How about I check here to make sure all the "compatible" strings are
> > from an approved transparent bus list, and if it's true, I use
> > driver_override to force match it to a transparent bus driver? Would
> > you be okay with that?
>
> Can't we do that within a driver? We check this and fail probe if
> there's a more specific compatible.  Then another driver can match and
> probe.

I was thinking that initially, but if we fail a probe, the driver core
will permanently give up (won't search further) or might end up
retrying with the same driver and never get to the other driver. I'll
send out a v2 with what I described above. It's not too bad and it
will also allow us to handle the PPC cases (we'll just need to keep
adding the simple-bus equivalent entries to a table).

-Saravana

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Rob Herring]

On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > We don't want fw_devlink creating device links for bus devices as
> > > > > they'll never probe. So mark those device node with this flag.
> > > > >
> > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > ---
> > > > >  drivers/of/platform.c | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > > index 74afbb7a4f5e..42b3936d204a 100644
> > > > > --- a/drivers/of/platform.c
> > > > > +++ b/drivers/of/platform.c
> > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > > > >         if (!dev || !of_match_node(matches, bus))
> > > > >                 return 0;
> > > > >
> > > > > +       /*
> > > > > +        * If the bus node has only one compatible string value and it has
> > > > > +        * matched as a bus node, it's never going to get probed by a device
> > > > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > > > +        * device links with this device.
> > > > > +        *
> > > > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > > > +        * enough for now.
> > > > > +        *
> > > > > +        * This doesn't really work for PPC because of how it uses
> > > > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > > > +        */
> > > > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > > > +           of_property_count_strings(bus, "compatible") == 1)
> > > > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> > > >
> > > > This looks fragile relying on 1 compatible string, and the DT flags in
> > > > this code have been fragile too. I'm pretty sure we have cases of
> > > > simple-bus or simple-mfd that also have another compatible.
> > > >
> > > > Couldn't we solve this with a simple driver?
> > >
> > > Oh, I didn't think you'd like that. I'd lean towards that option too
> > > if we can address some of the other concerns below.
> > >
> > > > Make 'simple-pm-bus'
> > > > driver work for other cases?
> > >
> > > > BTW, this patch doesn't even work for
> > > > simple-pm-bus.
> > >
> > > How do you mean? Because simple-pm-bus already has a driver and
> > > doesn't set "matches" param when it calls of_platform_populate() and
> > > this flag won't be set. So at least for simple-pm-bus I don't see any
> > > issue.
> >
> > You're right.
> >
> > > I was trying to reuse of_default_bus_match_table without explicitly
> > > referring to it, but if it's confusing I can add a separate list of
> > > compatible strings and use those here instead of using "matches".
> >
> > What happens with a non-default table? I'm not sure we can assume the
> > same behavior.
> >
> > > > A driver for simple-bus may cause issues if there's a
> > > > more specific driver to bind to as we don't handle that. It's simply
> > > > whichever matches first.
> > >
> > > Right, this is my worry. Especially for devices like this (there are
> > > plenty of cases like this) which have a driver that probes them but
> > > also lists simple-bus
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299
> >
> > Uhh, that one is certainly a leakage of wanting an soc_device in the
> > hierarchy, not any real bus structure reflecting the h/w. I'm not a
> > fan of the soc_device stuff and its optional nature. Everything is an
> > SoC, so it should always be there? Or your device hierarchy should
> > change when you decide to add a soc_device?
> >
> > > So as long as there's a compatible string that's not one of the
> > > "transparent" busses, this driver shouldn't match. So, I don't think I
> > > can get away from checking the compatible strings.
> > >
> > > How about I check here to make sure all the "compatible" strings are
> > > from an approved transparent bus list, and if it's true, I use
> > > driver_override to force match it to a transparent bus driver? Would
> > > you be okay with that?
> >
> > Can't we do that within a driver? We check this and fail probe if
> > there's a more specific compatible.  Then another driver can match and
> > probe.
>
> I was thinking that initially, but if we fail a probe, the driver core
> will permanently give up (won't search further) or might end up
> retrying with the same driver and never get to the other driver. I'll
> send out a v2 with what I described above. It's not too bad and it
> will also allow us to handle the PPC cases (we'll just need to keep
> adding the simple-bus equivalent entries to a table).

I wasn't sure, but I traced the calls and it looks like based on
__driver_attach() that if a driver fails probe another one matching
should get to probe:

        /*
         * Ignore errors returned by ->probe so that the next driver can try
         * its luck.
         */

The PPC case is about descending nodes without a compatible string.

Rob

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

On Thu, Sep 2, 2021 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > >
> > > > > > We don't want fw_devlink creating device links for bus devices as
> > > > > > they'll never probe. So mark those device node with this flag.
> > > > > >
> > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > ---
> > > > > >  drivers/of/platform.c | 16 ++++++++++++++++
> > > > > >  1 file changed, 16 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > > > index 74afbb7a4f5e..42b3936d204a 100644
> > > > > > --- a/drivers/of/platform.c
> > > > > > +++ b/drivers/of/platform.c
> > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > > > > >         if (!dev || !of_match_node(matches, bus))
> > > > > >                 return 0;
> > > > > >
> > > > > > +       /*
> > > > > > +        * If the bus node has only one compatible string value and it has
> > > > > > +        * matched as a bus node, it's never going to get probed by a device
> > > > > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > > > > +        * device links with this device.
> > > > > > +        *
> > > > > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > > > > +        * enough for now.
> > > > > > +        *
> > > > > > +        * This doesn't really work for PPC because of how it uses
> > > > > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > > > > +        */
> > > > > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > > > > +           of_property_count_strings(bus, "compatible") == 1)
> > > > > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> > > > >
> > > > > This looks fragile relying on 1 compatible string, and the DT flags in
> > > > > this code have been fragile too. I'm pretty sure we have cases of
> > > > > simple-bus or simple-mfd that also have another compatible.
> > > > >
> > > > > Couldn't we solve this with a simple driver?
> > > >
> > > > Oh, I didn't think you'd like that. I'd lean towards that option too
> > > > if we can address some of the other concerns below.
> > > >
> > > > > Make 'simple-pm-bus'
> > > > > driver work for other cases?
> > > >
> > > > > BTW, this patch doesn't even work for
> > > > > simple-pm-bus.
> > > >
> > > > How do you mean? Because simple-pm-bus already has a driver and
> > > > doesn't set "matches" param when it calls of_platform_populate() and
> > > > this flag won't be set. So at least for simple-pm-bus I don't see any
> > > > issue.
> > >
> > > You're right.
> > >
> > > > I was trying to reuse of_default_bus_match_table without explicitly
> > > > referring to it, but if it's confusing I can add a separate list of
> > > > compatible strings and use those here instead of using "matches".
> > >
> > > What happens with a non-default table? I'm not sure we can assume the
> > > same behavior.
> > >
> > > > > A driver for simple-bus may cause issues if there's a
> > > > > more specific driver to bind to as we don't handle that. It's simply
> > > > > whichever matches first.
> > > >
> > > > Right, this is my worry. Especially for devices like this (there are
> > > > plenty of cases like this) which have a driver that probes them but
> > > > also lists simple-bus
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299
> > >
> > > Uhh, that one is certainly a leakage of wanting an soc_device in the
> > > hierarchy, not any real bus structure reflecting the h/w. I'm not a
> > > fan of the soc_device stuff and its optional nature. Everything is an
> > > SoC, so it should always be there? Or your device hierarchy should
> > > change when you decide to add a soc_device?
> > >
> > > > So as long as there's a compatible string that's not one of the
> > > > "transparent" busses, this driver shouldn't match. So, I don't think I
> > > > can get away from checking the compatible strings.
> > > >
> > > > How about I check here to make sure all the "compatible" strings are
> > > > from an approved transparent bus list, and if it's true, I use
> > > > driver_override to force match it to a transparent bus driver? Would
> > > > you be okay with that?
> > >
> > > Can't we do that within a driver? We check this and fail probe if
> > > there's a more specific compatible.  Then another driver can match and
> > > probe.
> >
> > I was thinking that initially, but if we fail a probe, the driver core
> > will permanently give up (won't search further) or might end up
> > retrying with the same driver and never get to the other driver. I'll
> > send out a v2 with what I described above. It's not too bad and it
> > will also allow us to handle the PPC cases (we'll just need to keep
> > adding the simple-bus equivalent entries to a table).
>
> I wasn't sure, but I traced the calls and it looks like based on
> __driver_attach() that if a driver fails probe another one matching
> should get to probe:

__driver_attach() is called over every device already in a bus. It's
called only when a new driver is registered. So it makes sense that
one ignores the error returned from probe(). You don't want to fail
driver registration because one specific device needs to defer probe.

The comment is actually from __device_attach_driver()

>
>         /*
>          * Ignore errors returned by ->probe so that the next driver can try
>          * its luck.
>          */

I saw that comment too, but isn't the comment wrong/stale?

bus_probe_device() -> device_initial_probe() -> __device_attach().

In __device_attach() we have:
ret = bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);

If you look at bus_for_each_drv()'s comment:
 * ...... If @fn returns anything but 0, we break out
 * and return it. If @start is not NULL, we use it as the head
 * of the list.

Inside __device_attach_driver() we see:
        /*
         * Ignore errors returned by ->probe so that the next driver can try
         * its luck.
         */
        ret = driver_probe_device(drv, dev);
        if (ret < 0)
                return ret;

So if probe() returned an error, we'd return it right back out. And
then bus_for_each_drv() will stop searching for more drivers that
match.

So I don't think one driver can give up after a match and have another
driver give a device a shot.

>
> The PPC case is about descending nodes without a compatible string.

That whole thing is kinda confusing, but if PPC has more "simple"
busses, they can just add to the list I'm maintaining in the v2
series.

-Saravana

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Rob Herring]

On Thu, Sep 2, 2021 at 8:16 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Sep 2, 2021 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > >
> > > > > > > We don't want fw_devlink creating device links for bus devices as
> > > > > > > they'll never probe. So mark those device node with this flag.
> > > > > > >
> > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > ---
> > > > > > >  drivers/of/platform.c | 16 ++++++++++++++++
> > > > > > >  1 file changed, 16 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > > > > index 74afbb7a4f5e..42b3936d204a 100644
> > > > > > > --- a/drivers/of/platform.c
> > > > > > > +++ b/drivers/of/platform.c
> > > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > > > > > >         if (!dev || !of_match_node(matches, bus))
> > > > > > >                 return 0;
> > > > > > >
> > > > > > > +       /*
> > > > > > > +        * If the bus node has only one compatible string value and it has
> > > > > > > +        * matched as a bus node, it's never going to get probed by a device
> > > > > > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > > > > > +        * device links with this device.
> > > > > > > +        *
> > > > > > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > > > > > +        * enough for now.
> > > > > > > +        *
> > > > > > > +        * This doesn't really work for PPC because of how it uses
> > > > > > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > > > > > +        */
> > > > > > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > > > > > +           of_property_count_strings(bus, "compatible") == 1)
> > > > > > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> > > > > >
> > > > > > This looks fragile relying on 1 compatible string, and the DT flags in
> > > > > > this code have been fragile too. I'm pretty sure we have cases of
> > > > > > simple-bus or simple-mfd that also have another compatible.
> > > > > >
> > > > > > Couldn't we solve this with a simple driver?
> > > > >
> > > > > Oh, I didn't think you'd like that. I'd lean towards that option too
> > > > > if we can address some of the other concerns below.
> > > > >
> > > > > > Make 'simple-pm-bus'
> > > > > > driver work for other cases?
> > > > >
> > > > > > BTW, this patch doesn't even work for
> > > > > > simple-pm-bus.
> > > > >
> > > > > How do you mean? Because simple-pm-bus already has a driver and
> > > > > doesn't set "matches" param when it calls of_platform_populate() and
> > > > > this flag won't be set. So at least for simple-pm-bus I don't see any
> > > > > issue.
> > > >
> > > > You're right.
> > > >
> > > > > I was trying to reuse of_default_bus_match_table without explicitly
> > > > > referring to it, but if it's confusing I can add a separate list of
> > > > > compatible strings and use those here instead of using "matches".
> > > >
> > > > What happens with a non-default table? I'm not sure we can assume the
> > > > same behavior.
> > > >
> > > > > > A driver for simple-bus may cause issues if there's a
> > > > > > more specific driver to bind to as we don't handle that. It's simply
> > > > > > whichever matches first.
> > > > >
> > > > > Right, this is my worry. Especially for devices like this (there are
> > > > > plenty of cases like this) which have a driver that probes them but
> > > > > also lists simple-bus
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299
> > > >
> > > > Uhh, that one is certainly a leakage of wanting an soc_device in the
> > > > hierarchy, not any real bus structure reflecting the h/w. I'm not a
> > > > fan of the soc_device stuff and its optional nature. Everything is an
> > > > SoC, so it should always be there? Or your device hierarchy should
> > > > change when you decide to add a soc_device?
> > > >
> > > > > So as long as there's a compatible string that's not one of the
> > > > > "transparent" busses, this driver shouldn't match. So, I don't think I
> > > > > can get away from checking the compatible strings.
> > > > >
> > > > > How about I check here to make sure all the "compatible" strings are
> > > > > from an approved transparent bus list, and if it's true, I use
> > > > > driver_override to force match it to a transparent bus driver? Would
> > > > > you be okay with that?
> > > >
> > > > Can't we do that within a driver? We check this and fail probe if
> > > > there's a more specific compatible.  Then another driver can match and
> > > > probe.
> > >
> > > I was thinking that initially, but if we fail a probe, the driver core
> > > will permanently give up (won't search further) or might end up
> > > retrying with the same driver and never get to the other driver. I'll
> > > send out a v2 with what I described above. It's not too bad and it
> > > will also allow us to handle the PPC cases (we'll just need to keep
> > > adding the simple-bus equivalent entries to a table).
> >
> > I wasn't sure, but I traced the calls and it looks like based on
> > __driver_attach() that if a driver fails probe another one matching
> > should get to probe:
>
> __driver_attach() is called over every device already in a bus. It's
> called only when a new driver is registered. So it makes sense that
> one ignores the error returned from probe(). You don't want to fail
> driver registration because one specific device needs to defer probe.

The behavior should be the same no matter whether the device or driver
is registered first.

Deferred probe errors are handled differently AFAICT.

>
> The comment is actually from __device_attach_driver()
>
> >
> >         /*
> >          * Ignore errors returned by ->probe so that the next driver can try
> >          * its luck.
> >          */
>
> I saw that comment too, but isn't the comment wrong/stale?

I don't know...

>
> bus_probe_device() -> device_initial_probe() -> __device_attach().
>
> In __device_attach() we have:
> ret = bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);
>
> If you look at bus_for_each_drv()'s comment:
>  * ...... If @fn returns anything but 0, we break out
>  * and return it. If @start is not NULL, we use it as the head
>  * of the list.
>
> Inside __device_attach_driver() we see:
>         /*
>          * Ignore errors returned by ->probe so that the next driver can try
>          * its luck.
>          */
>         ret = driver_probe_device(drv, dev);
>         if (ret < 0)
>                 return ret;
>
> So if probe() returned an error, we'd return it right back out. And
> then bus_for_each_drv() will stop searching for more drivers that
> match.

With the exception of deferred probe, probe errors are made positive
and then ignored.

> So I don't think one driver can give up after a match and have another
> driver give a device a shot.

I think it just needs to be tried out...

I would like the above to work because it at least partially solves
the problem of multiple drivers matching. It only works if the
fallback driver knows there's a better match though which is this
case. If we don't know then a better match would have to unbind the
first driver and bind to the better match. I'm not sure that could
ever work generically.

Rob

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

On Fri, Sep 3, 2021 at 7:58 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Sep 2, 2021 at 8:16 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Sep 2, 2021 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > > >
> > > > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > > >
> > > > > > > > We don't want fw_devlink creating device links for bus devices as
> > > > > > > > they'll never probe. So mark those device node with this flag.
> > > > > > > >
> > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > > ---
> > > > > > > >  drivers/of/platform.c | 16 ++++++++++++++++
> > > > > > > >  1 file changed, 16 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > > > > > index 74afbb7a4f5e..42b3936d204a 100644
> > > > > > > > --- a/drivers/of/platform.c
> > > > > > > > +++ b/drivers/of/platform.c
> > > > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > > > > > > >         if (!dev || !of_match_node(matches, bus))
> > > > > > > >                 return 0;
> > > > > > > >
> > > > > > > > +       /*
> > > > > > > > +        * If the bus node has only one compatible string value and it has
> > > > > > > > +        * matched as a bus node, it's never going to get probed by a device
> > > > > > > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > > > > > > +        * device links with this device.
> > > > > > > > +        *
> > > > > > > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > > > > > > +        * enough for now.
> > > > > > > > +        *
> > > > > > > > +        * This doesn't really work for PPC because of how it uses
> > > > > > > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > > > > > > +        */
> > > > > > > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > > > > > > +           of_property_count_strings(bus, "compatible") == 1)
> > > > > > > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> > > > > > >
> > > > > > > This looks fragile relying on 1 compatible string, and the DT flags in
> > > > > > > this code have been fragile too. I'm pretty sure we have cases of
> > > > > > > simple-bus or simple-mfd that also have another compatible.
> > > > > > >
> > > > > > > Couldn't we solve this with a simple driver?
> > > > > >
> > > > > > Oh, I didn't think you'd like that. I'd lean towards that option too
> > > > > > if we can address some of the other concerns below.
> > > > > >
> > > > > > > Make 'simple-pm-bus'
> > > > > > > driver work for other cases?
> > > > > >
> > > > > > > BTW, this patch doesn't even work for
> > > > > > > simple-pm-bus.
> > > > > >
> > > > > > How do you mean? Because simple-pm-bus already has a driver and
> > > > > > doesn't set "matches" param when it calls of_platform_populate() and
> > > > > > this flag won't be set. So at least for simple-pm-bus I don't see any
> > > > > > issue.
> > > > >
> > > > > You're right.
> > > > >
> > > > > > I was trying to reuse of_default_bus_match_table without explicitly
> > > > > > referring to it, but if it's confusing I can add a separate list of
> > > > > > compatible strings and use those here instead of using "matches".
> > > > >
> > > > > What happens with a non-default table? I'm not sure we can assume the
> > > > > same behavior.
> > > > >
> > > > > > > A driver for simple-bus may cause issues if there's a
> > > > > > > more specific driver to bind to as we don't handle that. It's simply
> > > > > > > whichever matches first.
> > > > > >
> > > > > > Right, this is my worry. Especially for devices like this (there are
> > > > > > plenty of cases like this) which have a driver that probes them but
> > > > > > also lists simple-bus
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299
> > > > >
> > > > > Uhh, that one is certainly a leakage of wanting an soc_device in the
> > > > > hierarchy, not any real bus structure reflecting the h/w. I'm not a
> > > > > fan of the soc_device stuff and its optional nature. Everything is an
> > > > > SoC, so it should always be there? Or your device hierarchy should
> > > > > change when you decide to add a soc_device?
> > > > >
> > > > > > So as long as there's a compatible string that's not one of the
> > > > > > "transparent" busses, this driver shouldn't match. So, I don't think I
> > > > > > can get away from checking the compatible strings.
> > > > > >
> > > > > > How about I check here to make sure all the "compatible" strings are
> > > > > > from an approved transparent bus list, and if it's true, I use
> > > > > > driver_override to force match it to a transparent bus driver? Would
> > > > > > you be okay with that?
> > > > >
> > > > > Can't we do that within a driver? We check this and fail probe if
> > > > > there's a more specific compatible.  Then another driver can match and
> > > > > probe.
> > > >
> > > > I was thinking that initially, but if we fail a probe, the driver core
> > > > will permanently give up (won't search further) or might end up
> > > > retrying with the same driver and never get to the other driver. I'll
> > > > send out a v2 with what I described above. It's not too bad and it
> > > > will also allow us to handle the PPC cases (we'll just need to keep
> > > > adding the simple-bus equivalent entries to a table).
> > >
> > > I wasn't sure, but I traced the calls and it looks like based on
> > > __driver_attach() that if a driver fails probe another one matching
> > > should get to probe:
> >
> > __driver_attach() is called over every device already in a bus. It's
> > called only when a new driver is registered. So it makes sense that
> > one ignores the error returned from probe(). You don't want to fail
> > driver registration because one specific device needs to defer probe.
>
> The behavior should be the same no matter whether the device or driver
> is registered first.
>
> Deferred probe errors are handled differently AFAICT.
>
> >
> > The comment is actually from __device_attach_driver()
> >
> > >
> > >         /*
> > >          * Ignore errors returned by ->probe so that the next driver can try
> > >          * its luck.
> > >          */
> >
> > I saw that comment too, but isn't the comment wrong/stale?
>
> I don't know...
>
> >
> > bus_probe_device() -> device_initial_probe() -> __device_attach().
> >
> > In __device_attach() we have:
> > ret = bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);
> >
> > If you look at bus_for_each_drv()'s comment:
> >  * ...... If @fn returns anything but 0, we break out
> >  * and return it. If @start is not NULL, we use it as the head
> >  * of the list.
> >
> > Inside __device_attach_driver() we see:
> >         /*
> >          * Ignore errors returned by ->probe so that the next driver can try
> >          * its luck.
> >          */
> >         ret = driver_probe_device(drv, dev);
> >         if (ret < 0)
> >                 return ret;
> >
> > So if probe() returned an error, we'd return it right back out. And
> > then bus_for_each_drv() will stop searching for more drivers that
> > match.
>
> With the exception of deferred probe, probe errors are made positive
> and then ignored.

Oh I totally missed this!

>
> > So I don't think one driver can give up after a match and have another
> > driver give a device a shot.
>
> I think it just needs to be tried out...

Oh yeah, it would definitely work and would be a lot nicer. I'll rework this.

-Saravana

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Rafael J. Wysocki]

On Fri, Sep 3, 2021 at 3:16 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Sep 2, 2021 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > >
> > > > > > > We don't want fw_devlink creating device links for bus devices as
> > > > > > > they'll never probe. So mark those device node with this flag.
> > > > > > >
> > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > ---
> > > > > > >  drivers/of/platform.c | 16 ++++++++++++++++
> > > > > > >  1 file changed, 16 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > > > > > index 74afbb7a4f5e..42b3936d204a 100644
> > > > > > > --- a/drivers/of/platform.c
> > > > > > > +++ b/drivers/of/platform.c
> > > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
> > > > > > >         if (!dev || !of_match_node(matches, bus))
> > > > > > >                 return 0;
> > > > > > >
> > > > > > > +       /*
> > > > > > > +        * If the bus node has only one compatible string value and it has
> > > > > > > +        * matched as a bus node, it's never going to get probed by a device
> > > > > > > +        * driver. So flag it as such so that fw_devlink knows not to create
> > > > > > > +        * device links with this device.
> > > > > > > +        *
> > > > > > > +        * This doesn't catch all devices that'll never probe, but this is good
> > > > > > > +        * enough for now.
> > > > > > > +        *
> > > > > > > +        * This doesn't really work for PPC because of how it uses
> > > > > > > +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> > > > > > > +        */
> > > > > > > +       if (!IS_ENABLED(CONFIG_PPC) &&
> > > > > > > +           of_property_count_strings(bus, "compatible") == 1)
> > > > > > > +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> > > > > >
> > > > > > This looks fragile relying on 1 compatible string, and the DT flags in
> > > > > > this code have been fragile too. I'm pretty sure we have cases of
> > > > > > simple-bus or simple-mfd that also have another compatible.
> > > > > >
> > > > > > Couldn't we solve this with a simple driver?
> > > > >
> > > > > Oh, I didn't think you'd like that. I'd lean towards that option too
> > > > > if we can address some of the other concerns below.
> > > > >
> > > > > > Make 'simple-pm-bus'
> > > > > > driver work for other cases?
> > > > >
> > > > > > BTW, this patch doesn't even work for
> > > > > > simple-pm-bus.
> > > > >
> > > > > How do you mean? Because simple-pm-bus already has a driver and
> > > > > doesn't set "matches" param when it calls of_platform_populate() and
> > > > > this flag won't be set. So at least for simple-pm-bus I don't see any
> > > > > issue.
> > > >
> > > > You're right.
> > > >
> > > > > I was trying to reuse of_default_bus_match_table without explicitly
> > > > > referring to it, but if it's confusing I can add a separate list of
> > > > > compatible strings and use those here instead of using "matches".
> > > >
> > > > What happens with a non-default table? I'm not sure we can assume the
> > > > same behavior.
> > > >
> > > > > > A driver for simple-bus may cause issues if there's a
> > > > > > more specific driver to bind to as we don't handle that. It's simply
> > > > > > whichever matches first.
> > > > >
> > > > > Right, this is my worry. Especially for devices like this (there are
> > > > > plenty of cases like this) which have a driver that probes them but
> > > > > also lists simple-bus
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299
> > > >
> > > > Uhh, that one is certainly a leakage of wanting an soc_device in the
> > > > hierarchy, not any real bus structure reflecting the h/w. I'm not a
> > > > fan of the soc_device stuff and its optional nature. Everything is an
> > > > SoC, so it should always be there? Or your device hierarchy should
> > > > change when you decide to add a soc_device?
> > > >
> > > > > So as long as there's a compatible string that's not one of the
> > > > > "transparent" busses, this driver shouldn't match. So, I don't think I
> > > > > can get away from checking the compatible strings.
> > > > >
> > > > > How about I check here to make sure all the "compatible" strings are
> > > > > from an approved transparent bus list, and if it's true, I use
> > > > > driver_override to force match it to a transparent bus driver? Would
> > > > > you be okay with that?
> > > >
> > > > Can't we do that within a driver? We check this and fail probe if
> > > > there's a more specific compatible.  Then another driver can match and
> > > > probe.
> > >
> > > I was thinking that initially, but if we fail a probe, the driver core
> > > will permanently give up (won't search further) or might end up
> > > retrying with the same driver and never get to the other driver. I'll
> > > send out a v2 with what I described above. It's not too bad and it
> > > will also allow us to handle the PPC cases (we'll just need to keep
> > > adding the simple-bus equivalent entries to a table).
> >
> > I wasn't sure, but I traced the calls and it looks like based on
> > __driver_attach() that if a driver fails probe another one matching
> > should get to probe:
>
> __driver_attach() is called over every device already in a bus. It's
> called only when a new driver is registered. So it makes sense that
> one ignores the error returned from probe(). You don't want to fail
> driver registration because one specific device needs to defer probe.
>
> The comment is actually from __device_attach_driver()
>
> >
> >         /*
> >          * Ignore errors returned by ->probe so that the next driver can try
> >          * its luck.
> >          */
>
> I saw that comment too, but isn't the comment wrong/stale?
>
> bus_probe_device() -> device_initial_probe() -> __device_attach().
>
> In __device_attach() we have:
> ret = bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);
>
> If you look at bus_for_each_drv()'s comment:
>  * ...... If @fn returns anything but 0, we break out
>  * and return it. If @start is not NULL, we use it as the head
>  * of the list.
>
> Inside __device_attach_driver() we see:
>         /*
>          * Ignore errors returned by ->probe so that the next driver can try
>          * its luck.
>          */
>         ret = driver_probe_device(drv, dev);
>         if (ret < 0)
>                 return ret;
>
> So if probe() returned an error, we'd return it right back out. And
> then bus_for_each_drv() will stop searching for more drivers that
> match.

Well, not quite.

If ->probe() returns an error, really_probe() will convert it into a
positive number.  __driver_probe_device() will then return as is and
driver_probe_device() doesn't touch that value.

Bottom line: you'll see a positive number here, so the check above
will not trigger and 0 is returned, so bus_for_each_drv() will
actually continue searching.

> So I don't think one driver can give up after a match and have another
> driver give a device a shot.

Yes, it can.

Re: [PATCH v1 2/2] of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES

[Saravana Kannan]

On Wed, Sep 1, 2021 at 7:55 PM Saravana Kannan <saravanak@google.com> wrote:
>
> We don't want fw_devlink creating device links for bus devices as
> they'll never probe. So mark those device node with this flag.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/platform.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 74afbb7a4f5e..42b3936d204a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus,
>         if (!dev || !of_match_node(matches, bus))
>                 return 0;
>
> +       /*
> +        * If the bus node has only one compatible string value and it has
> +        * matched as a bus node, it's never going to get probed by a device
> +        * driver. So flag it as such so that fw_devlink knows not to create
> +        * device links with this device.
> +        *
> +        * This doesn't catch all devices that'll never probe, but this is good
> +        * enough for now.
> +        *
> +        * This doesn't really work for PPC because of how it uses
> +        * of_platform_bus_probe() to add normal devices. So ignore PPC cases.
> +        */
> +       if (!IS_ENABLED(CONFIG_PPC) &&
> +           of_property_count_strings(bus, "compatible") == 1)
> +               bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;

This patch is wrong. This flag should have been FWNODE_FLAG_NEVER_PROBES.

-Saravana

> +
>         for_each_child_of_node(bus, child) {
>                 pr_debug("   create child: %pOF\n", child);
>                 rc = of_platform_bus_create(child, matches, lookup, &dev->dev, strict);
> --
> 2.33.0.259.gc128427fd7-goog
>

Re: [PATCH v1 0/2] Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

[Saravana Kannan]

Oops, forgot to use a proper subject. Sorry.

-Saravana

On Wed, Sep 1, 2021 at 7:55 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Ulf, mind testing this?
>
> Thanks,
> Saravana
> [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>
> Saravana Kannan (2):
>   driver core: Add support for FWNODE_FLAG_NEVER_PROBES
>   of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES
>
>  drivers/base/core.c    |  8 ++++++++
>  drivers/of/platform.c  | 16 ++++++++++++++++
>  include/linux/fwnode.h |  8 +++++---
>  3 files changed, 29 insertions(+), 3 deletions(-)
>
> --
> 2.33.0.259.gc128427fd7-goog
>

Re: [PATCH v1 0/2] Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

[Rafael J. Wysocki]

On Thu, Sep 2, 2021 at 4:57 AM Saravana Kannan <saravanak@google.com> wrote:
>
> Oops, forgot to use a proper subject. Sorry.

Is this a replacement for the "Fix rtl8366rb issues with fw_devlink=on " series?


> On Wed, Sep 1, 2021 at 7:55 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Ulf, mind testing this?
> >
> > Thanks,
> > Saravana
> > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> >
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >
> > Saravana Kannan (2):
> >   driver core: Add support for FWNODE_FLAG_NEVER_PROBES
> >   of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES
> >
> >  drivers/base/core.c    |  8 ++++++++
> >  drivers/of/platform.c  | 16 ++++++++++++++++
> >  include/linux/fwnode.h |  8 +++++---
> >  3 files changed, 29 insertions(+), 3 deletions(-)
> >
> > --
> > 2.33.0.259.gc128427fd7-goog
> >

Re: [PATCH v1 0/2] Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

[Saravana Kannan]

On Thu, Sep 2, 2021 at 8:56 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Sep 2, 2021 at 4:57 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Oops, forgot to use a proper subject. Sorry.
>
> Is this a replacement for the "Fix rtl8366rb issues with fw_devlink=on " series?

No. This is unrelated to that. This is the issue I'm trying to fix:
https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

This is kind of a replacement to, but the patch below might be needed
in general (needs more thought):
https://lore.kernel.org/lkml/CAGETcx9U2M5i1CAx605fG3Qwm1xwjH2uy4kY4vrAF7YSRSSg+w@mail.gmail.com/

-Saravana
>
>
> > On Wed, Sep 1, 2021 at 7:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Ulf, mind testing this?
> > >
> > > Thanks,
> > > Saravana
> > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > >
> > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > >
> > > Saravana Kannan (2):
> > >   driver core: Add support for FWNODE_FLAG_NEVER_PROBES
> > >   of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES
> > >
> > >  drivers/base/core.c    |  8 ++++++++
> > >  drivers/of/platform.c  | 16 ++++++++++++++++
> > >  include/linux/fwnode.h |  8 +++++---
> > >  3 files changed, 29 insertions(+), 3 deletions(-)
> > >
> > > --
> > > 2.33.0.259.gc128427fd7-goog
> > >

Re: [PATCH v1 0/2] Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

[Rafael J. Wysocki]

On Thu, Sep 2, 2021 at 6:27 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Sep 2, 2021 at 8:56 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Sep 2, 2021 at 4:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Oops, forgot to use a proper subject. Sorry.
> >
> > Is this a replacement for the "Fix rtl8366rb issues with fw_devlink=on " series?
>
> No. This is unrelated to that. This is the issue I'm trying to fix:
> https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Allright, thanks!

> This is kind of a replacement to, but the patch below might be needed
> in general (needs more thought):
> https://lore.kernel.org/lkml/CAGETcx9U2M5i1CAx605fG3Qwm1xwjH2uy4kY4vrAF7YSRSSg+w@mail.gmail.com/

I see.


> > > On Wed, Sep 1, 2021 at 7:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > Ulf, mind testing this?
> > > >
> > > > Thanks,
> > > > Saravana
> > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > >
> > > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > >
> > > > Saravana Kannan (2):
> > > >   driver core: Add support for FWNODE_FLAG_NEVER_PROBES
> > > >   of: platform: Mark bus devices nodes with FWNODE_FLAG_NEVER_PROBES
> > > >
> > > >  drivers/base/core.c    |  8 ++++++++
> > > >  drivers/of/platform.c  | 16 ++++++++++++++++
> > > >  include/linux/fwnode.h |  8 +++++---
> > > >  3 files changed, 29 insertions(+), 3 deletions(-)
> > > >
> > > > --
> > > > 2.33.0.259.gc128427fd7-goog
> > > >