[PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Saravana Kannan]

Devices that are not on a bus will never have a driver bound to it. So,
fw_devlink should not create device links for it as it can cause probe
issues[1] or sync_state() call back issues[2].

[1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
[2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index f6360490a4a3..304a06314656 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
 	struct device *sup_dev;
 	int ret = 0;
 
+	/*
+	 * If a consumer device is not on a bus (i.e. a driver will never bind
+	 * to it), it doesn't make sense for fw_devlink to create device links
+	 * for it.
+	 */
+	if (con->bus == NULL)
+		return -EINVAL;
+
 	sup_dev = get_dev_from_fwnode(sup_handle);
 	if (sup_dev) {
+		/*
+		 * If a supplier device is not on a bus (i.e. a driver will
+		 * never bind to it), it doesn't make sense for fw_devlink to
+		 * create device links for it.
+		 */
+		if (sup_dev->bus == NULL)
+			return -EINVAL;
+
 		/*
 		 * If it's one of those drivers that don't actually bind to
 		 * their device using driver core, then don't wait on this
-- 
2.33.0.259.gc128427fd7-goog

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Greg Kroah-Hartman]

On Tue, Aug 31, 2021 at 03:45:10PM -0700, Saravana Kannan wrote:
> Devices that are not on a bus will never have a driver bound to it. So,
> fw_devlink should not create device links for it as it can cause probe
> issues[1] or sync_state() call back issues[2].
> 
> [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f6360490a4a3..304a06314656 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
>  	struct device *sup_dev;
>  	int ret = 0;
>  
> +	/*
> +	 * If a consumer device is not on a bus (i.e. a driver will never bind
> +	 * to it), it doesn't make sense for fw_devlink to create device links
> +	 * for it.
> +	 */
> +	if (con->bus == NULL)
> +		return -EINVAL;
> +

Why would a device not be on a bus that has to do with a driver needing
it?  What types of devices are these?

thanks,

greg k-h

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Saravana Kannan]

On Tue, Aug 31, 2021 at 11:24 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Aug 31, 2021 at 03:45:10PM -0700, Saravana Kannan wrote:
> > Devices that are not on a bus will never have a driver bound to it. So,
> > fw_devlink should not create device links for it as it can cause probe
> > issues[1] or sync_state() call back issues[2].
> >
> > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index f6360490a4a3..304a06314656 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
> >       struct device *sup_dev;
> >       int ret = 0;
> >
> > +     /*
> > +      * If a consumer device is not on a bus (i.e. a driver will never bind
> > +      * to it), it doesn't make sense for fw_devlink to create device links
> > +      * for it.
> > +      */
> > +     if (con->bus == NULL)
> > +             return -EINVAL;
> > +
>
> Why would a device not be on a bus that has to do with a driver needing
> it?  What types of devices are these?

I'm not sure what you are asking, because it seems like a question
you'd know the answer to. What I'm trying to say is that
bus_probe_device() is obviously only possible if the device is on a
bus. And fw_devlink creates managed device links that track if a
device is bound to a driver. So it doesn't make sense to create the
device link if the device is not on a bus.

There are plenty of examples for this -- like all the devices that get
added to a class? For example the platform bus or the mdio bus itself
are devices that are added to a class instead of a bus.

Not sure if I answered your question. Let me know.

-Saravana

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Greg Kroah-Hartman]

On Tue, Aug 31, 2021 at 11:57:45PM -0700, Saravana Kannan wrote:
> On Tue, Aug 31, 2021 at 11:24 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Aug 31, 2021 at 03:45:10PM -0700, Saravana Kannan wrote:
> > > Devices that are not on a bus will never have a driver bound to it. So,
> > > fw_devlink should not create device links for it as it can cause probe
> > > issues[1] or sync_state() call back issues[2].
> > >
> > > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> > > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index f6360490a4a3..304a06314656 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
> > >       struct device *sup_dev;
> > >       int ret = 0;
> > >
> > > +     /*
> > > +      * If a consumer device is not on a bus (i.e. a driver will never bind
> > > +      * to it), it doesn't make sense for fw_devlink to create device links
> > > +      * for it.
> > > +      */
> > > +     if (con->bus == NULL)
> > > +             return -EINVAL;
> > > +
> >
> > Why would a device not be on a bus that has to do with a driver needing
> > it?  What types of devices are these?
> 
> I'm not sure what you are asking, because it seems like a question
> you'd know the answer to. What I'm trying to say is that
> bus_probe_device() is obviously only possible if the device is on a
> bus. And fw_devlink creates managed device links that track if a
> device is bound to a driver. So it doesn't make sense to create the
> device link if the device is not on a bus.
> 
> There are plenty of examples for this -- like all the devices that get
> added to a class? For example the platform bus or the mdio bus itself
> are devices that are added to a class instead of a bus.
> 
> Not sure if I answered your question. Let me know.

Sorry, that was a vague question.

I was trying to say, "how can a device that is not attached to a bus at
all even be called for this?"  Who would be making that type of
connection, what subsystem is causing this to happen?

thanks,

greg k-h

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Ulf Hansson]

On Wed, 1 Sept 2021 at 00:45, Saravana Kannan <saravanak@google.com> wrote:
>
> Devices that are not on a bus will never have a driver bound to it. So,
> fw_devlink should not create device links for it as it can cause probe
> issues[1] or sync_state() call back issues[2].
>
> [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Unfortunately, this doesn't fix my problem in [2].

When the "soctest" device is initialized, via of_platform_populate(),
it will be attached to the platform bus, hence the check for the bus
pointer that you suggest to add below, doesn't have an impact on my
use case. I still get the below in the log:

"platform soctest: Linked as a sync state only consumer to pm_domain_test"

Kind regards
Uffe

> Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f6360490a4a3..304a06314656 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
>         struct device *sup_dev;
>         int ret = 0;
>
> +       /*
> +        * If a consumer device is not on a bus (i.e. a driver will never bind
> +        * to it), it doesn't make sense for fw_devlink to create device links
> +        * for it.
> +        */
> +       if (con->bus == NULL)
> +               return -EINVAL;
> +
>         sup_dev = get_dev_from_fwnode(sup_handle);
>         if (sup_dev) {
> +               /*
> +                * If a supplier device is not on a bus (i.e. a driver will
> +                * never bind to it), it doesn't make sense for fw_devlink to
> +                * create device links for it.
> +                */
> +               if (sup_dev->bus == NULL)
> +                       return -EINVAL;
> +
>                 /*
>                  * If it's one of those drivers that don't actually bind to
>                  * their device using driver core, then don't wait on this
> --
> 2.33.0.259.gc128427fd7-goog
>

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Saravana Kannan]

On Wed, Sep 1, 2021 at 8:45 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 1 Sept 2021 at 00:45, Saravana Kannan <saravanak@google.com> wrote:
> >
> > Devices that are not on a bus will never have a driver bound to it. So,
> > fw_devlink should not create device links for it as it can cause probe
> > issues[1] or sync_state() call back issues[2].
> >
> > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
>
> Unfortunately, this doesn't fix my problem in [2].
>
> When the "soctest" device is initialized, via of_platform_populate(),
> it will be attached to the platform bus, hence the check for the bus
> pointer that you suggest to add below, doesn't have an impact on my
> use case. I still get the below in the log:

*face palm* Right. I forgot that. I just read "bus" and my mind went
to busses added as devices. It apparently also didn't help [1] which
is surprising to me. I'll dig into that separately. I'll look into
fixing this. The annoying part is that some devices have compatible
property that's both "simple-bus" and some other string that a driver
actually matches with.

-Saravana

>
> "platform soctest: Linked as a sync state only consumer to pm_domain_test"
>
> Kind regards
> Uffe
>
> > Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/base/core.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index f6360490a4a3..304a06314656 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
> >         struct device *sup_dev;
> >         int ret = 0;
> >
> > +       /*
> > +        * If a consumer device is not on a bus (i.e. a driver will never bind
> > +        * to it), it doesn't make sense for fw_devlink to create device links
> > +        * for it.
> > +        */
> > +       if (con->bus == NULL)
> > +               return -EINVAL;
> > +
> >         sup_dev = get_dev_from_fwnode(sup_handle);
> >         if (sup_dev) {
> > +               /*
> > +                * If a supplier device is not on a bus (i.e. a driver will
> > +                * never bind to it), it doesn't make sense for fw_devlink to
> > +                * create device links for it.
> > +                */
> > +               if (sup_dev->bus == NULL)
> > +                       return -EINVAL;
> > +
> >                 /*
> >                  * If it's one of those drivers that don't actually bind to
> >                  * their device using driver core, then don't wait on this
> > --
> > 2.33.0.259.gc128427fd7-goog
> >

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Saravana Kannan]

On Wed, Sep 1, 2021 at 12:14 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Aug 31, 2021 at 11:57:45PM -0700, Saravana Kannan wrote:
> > On Tue, Aug 31, 2021 at 11:24 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Aug 31, 2021 at 03:45:10PM -0700, Saravana Kannan wrote:
> > > > Devices that are not on a bus will never have a driver bound to it. So,
> > > > fw_devlink should not create device links for it as it can cause probe
> > > > issues[1] or sync_state() call back issues[2].
> > > >
> > > > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > > > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > > Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> > > > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/base/core.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index f6360490a4a3..304a06314656 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
> > > >       struct device *sup_dev;
> > > >       int ret = 0;
> > > >
> > > > +     /*
> > > > +      * If a consumer device is not on a bus (i.e. a driver will never bind
> > > > +      * to it), it doesn't make sense for fw_devlink to create device links
> > > > +      * for it.
> > > > +      */
> > > > +     if (con->bus == NULL)
> > > > +             return -EINVAL;
> > > > +
> > >
> > > Why would a device not be on a bus that has to do with a driver needing
> > > it?  What types of devices are these?
> >
> > I'm not sure what you are asking, because it seems like a question
> > you'd know the answer to. What I'm trying to say is that
> > bus_probe_device() is obviously only possible if the device is on a
> > bus. And fw_devlink creates managed device links that track if a
> > device is bound to a driver. So it doesn't make sense to create the
> > device link if the device is not on a bus.
> >
> > There are plenty of examples for this -- like all the devices that get
> > added to a class? For example the platform bus or the mdio bus itself
> > are devices that are added to a class instead of a bus.
> >
> > Not sure if I answered your question. Let me know.
>
> Sorry, that was a vague question.
>
> I was trying to say, "how can a device that is not attached to a bus at
> all even be called for this?"

This code path is called for all device_add()s.

> Who would be making that type of
> connection, what subsystem is causing this to happen?

fw_devlink trying to interpret DT dependencies can sometimes end up in
this situation for some uncommon cases. But it's not helping [1] or
[2], so for now let's drop this patch. I'll come back to this if I
need this to fix any real issue.

-Saravana

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Ulf Hansson]

[...]

> fw_devlink trying to interpret DT dependencies can sometimes end up in
> this situation for some uncommon cases. But it's not helping [1] or
> [2], so for now let's drop this patch. I'll come back to this if I
> need this to fix any real issue.

Just to be clear, the issue is really there, with the psci power
domain, for example.

I am working with an ARM vendor on a platform that has a child node
below the common soc node and which child node is a consumer of the
psci power domain provider node. We fail to get the ->sync_state() to
be called, for reasons that have been explained by now, I think.

The reason for explaining the problem with a superficial example was
to try to easier point out the problem.

That said, I would certainly appreciate your help to get this fixed,
in one way or the other.

>
> -Saravana

Kind regards
Uffe

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Ulf Hansson]

On Wed, 1 Sept 2021 at 23:54, Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Sep 1, 2021 at 8:45 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 1 Sept 2021 at 00:45, Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Devices that are not on a bus will never have a driver bound to it. So,
> > > fw_devlink should not create device links for it as it can cause probe
> > > issues[1] or sync_state() call back issues[2].
> > >
> > > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> >
> > Unfortunately, this doesn't fix my problem in [2].
> >
> > When the "soctest" device is initialized, via of_platform_populate(),
> > it will be attached to the platform bus, hence the check for the bus
> > pointer that you suggest to add below, doesn't have an impact on my
> > use case. I still get the below in the log:
>
> *face palm* Right. I forgot that. I just read "bus" and my mind went
> to busses added as devices. It apparently also didn't help [1] which
> is surprising to me. I'll dig into that separately. I'll look into
> fixing this. The annoying part is that some devices have compatible
> property that's both "simple-bus" and some other string that a driver
> actually matches with.

Yes, that is my view of the problem as well.

So perhaps we should do a more fine grained check for when the
"simple-bus" compatible is present in the node, and then don't create
a fw_devlink if we reach an ascendant with only this compatible?

Would that work, do you think?

Kind regards
Uffe

>
> -Saravana
>
> >
> > "platform soctest: Linked as a sync state only consumer to pm_domain_test"
> >
> > Kind regards
> > Uffe
> >
> > > Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
> > > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/base/core.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index f6360490a4a3..304a06314656 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -1719,8 +1719,24 @@ static int fw_devlink_create_devlink(struct device *con,
> > >         struct device *sup_dev;
> > >         int ret = 0;
> > >
> > > +       /*
> > > +        * If a consumer device is not on a bus (i.e. a driver will never bind
> > > +        * to it), it doesn't make sense for fw_devlink to create device links
> > > +        * for it.
> > > +        */
> > > +       if (con->bus == NULL)
> > > +               return -EINVAL;
> > > +
> > >         sup_dev = get_dev_from_fwnode(sup_handle);
> > >         if (sup_dev) {
> > > +               /*
> > > +                * If a supplier device is not on a bus (i.e. a driver will
> > > +                * never bind to it), it doesn't make sense for fw_devlink to
> > > +                * create device links for it.
> > > +                */
> > > +               if (sup_dev->bus == NULL)
> > > +                       return -EINVAL;
> > > +
> > >                 /*
> > >                  * If it's one of those drivers that don't actually bind to
> > >                  * their device using driver core, then don't wait on this
> > > --
> > > 2.33.0.259.gc128427fd7-goog
> > >

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Saravana Kannan]

On Wed, Sep 1, 2021 at 3:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 1 Sept 2021 at 23:54, Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Sep 1, 2021 at 8:45 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 1 Sept 2021 at 00:45, Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > Devices that are not on a bus will never have a driver bound to it. So,
> > > > fw_devlink should not create device links for it as it can cause probe
> > > > issues[1] or sync_state() call back issues[2].
> > > >
> > > > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > > > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > >
> > > Unfortunately, this doesn't fix my problem in [2].
> > >
> > > When the "soctest" device is initialized, via of_platform_populate(),
> > > it will be attached to the platform bus, hence the check for the bus
> > > pointer that you suggest to add below, doesn't have an impact on my
> > > use case. I still get the below in the log:
> >
> > *face palm* Right. I forgot that. I just read "bus" and my mind went
> > to busses added as devices. It apparently also didn't help [1] which
> > is surprising to me. I'll dig into that separately. I'll look into
> > fixing this. The annoying part is that some devices have compatible
> > property that's both "simple-bus" and some other string that a driver
> > actually matches with.
>
> Yes, that is my view of the problem as well.
>
> So perhaps we should do a more fine grained check for when the
> "simple-bus" compatible is present in the node,

Exactly. Do you want to take a stab at this? There are too many things
I want to work on, so if you can do this one, that'd be nice.

> and then don't create
> a fw_devlink if we reach an ascendant with only this compatible?

And you can achieve that by setting this flag for that DT node:
fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;

-Saravana

Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus

[Saravana Kannan]

On Wed, Sep 1, 2021 at 5:16 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Sep 1, 2021 at 3:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 1 Sept 2021 at 23:54, Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Wed, Sep 1, 2021 at 8:45 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 1 Sept 2021 at 00:45, Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > Devices that are not on a bus will never have a driver bound to it. So,
> > > > > fw_devlink should not create device links for it as it can cause probe
> > > > > issues[1] or sync_state() call back issues[2].
> > > > >
> > > > > [1] - https://lore.kernel.org/lkml/CAGETcx_xJCqOWtwZ9Ee2+0sPGNLM5=F=djtbdYENkAYZa0ynqQ@mail.gmail.com/
> > > > > [2] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
> > > >
> > > > Unfortunately, this doesn't fix my problem in [2].
> > > >
> > > > When the "soctest" device is initialized, via of_platform_populate(),
> > > > it will be attached to the platform bus, hence the check for the bus
> > > > pointer that you suggest to add below, doesn't have an impact on my
> > > > use case. I still get the below in the log:
> > >
> > > *face palm* Right. I forgot that. I just read "bus" and my mind went
> > > to busses added as devices. It apparently also didn't help [1] which
> > > is surprising to me. I'll dig into that separately. I'll look into
> > > fixing this. The annoying part is that some devices have compatible
> > > property that's both "simple-bus" and some other string that a driver
> > > actually matches with.
> >
> > Yes, that is my view of the problem as well.
> >
> > So perhaps we should do a more fine grained check for when the
> > "simple-bus" compatible is present in the node,
>
> Exactly. Do you want to take a stab at this? There are too many things
> I want to work on, so if you can do this one, that'd be nice.

Nevermind. I already did most of it and it needs a little bit more
massaging. I'll send out a patch later.

-Saravana

>
> > and then don't create
> > a fw_devlink if we reach an ascendant with only this compatible?
>
> And you can achieve that by setting this flag for that DT node:
> fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
>
> -Saravana