* [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
@ 2021-08-31 22:45 Saravana Kannan
2021-09-01 6:24 ` Greg Kroah-Hartman
2021-09-01 15:44 ` Ulf Hansson
0 siblings, 2 replies; 11+ messages in thread
From: Saravana Kannan @ 2021-08-31 22:45 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: Ulf Hansson, Marek Szyprowski, kernel-team, linux-kernel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-08-31 22:45 [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus Saravana Kannan
@ 2021-09-01 6:24 ` Greg Kroah-Hartman
2021-09-01 6:57 ` Saravana Kannan
2021-09-01 15:44 ` Ulf Hansson
1 sibling, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-01 6:24 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski, kernel-team,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 6:24 ` Greg Kroah-Hartman
@ 2021-09-01 6:57 ` Saravana Kannan
2021-09-01 7:14 ` Greg Kroah-Hartman
0 siblings, 1 reply; 11+ messages in thread
From: Saravana Kannan @ 2021-09-01 6:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski, kernel-team,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 6:57 ` Saravana Kannan
@ 2021-09-01 7:14 ` Greg Kroah-Hartman
2021-09-01 21:55 ` Saravana Kannan
0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-01 7:14 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski, kernel-team,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-08-31 22:45 [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus Saravana Kannan
2021-09-01 6:24 ` Greg Kroah-Hartman
@ 2021-09-01 15:44 ` Ulf Hansson
2021-09-01 21:53 ` Saravana Kannan
1 sibling, 1 reply; 11+ messages in thread
From: Ulf Hansson @ 2021-09-01 15:44 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Marek Szyprowski,
Android Kernel Team, Linux Kernel Mailing List
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 15:44 ` Ulf Hansson
@ 2021-09-01 21:53 ` Saravana Kannan
2021-09-01 22:17 ` Ulf Hansson
0 siblings, 1 reply; 11+ messages in thread
From: Saravana Kannan @ 2021-09-01 21:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Marek Szyprowski,
Android Kernel Team, Linux Kernel Mailing List
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
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 7:14 ` Greg Kroah-Hartman
@ 2021-09-01 21:55 ` Saravana Kannan
2021-09-01 22:11 ` Ulf Hansson
0 siblings, 1 reply; 11+ messages in thread
From: Saravana Kannan @ 2021-09-01 21:55 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski, kernel-team,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 21:55 ` Saravana Kannan
@ 2021-09-01 22:11 ` Ulf Hansson
0 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2021-09-01 22:11 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Marek Szyprowski,
Android Kernel Team, Linux Kernel Mailing List
[...]
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 21:53 ` Saravana Kannan
@ 2021-09-01 22:17 ` Ulf Hansson
2021-09-02 0:16 ` Saravana Kannan
0 siblings, 1 reply; 11+ messages in thread
From: Ulf Hansson @ 2021-09-01 22:17 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Marek Szyprowski,
Android Kernel Team, Linux Kernel Mailing List
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
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-01 22:17 ` Ulf Hansson
@ 2021-09-02 0:16 ` Saravana Kannan
2021-09-02 1:43 ` Saravana Kannan
0 siblings, 1 reply; 11+ messages in thread
From: Saravana Kannan @ 2021-09-02 0:16 UTC (permalink / raw)
To: Ulf Hansson
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Marek Szyprowski,
Android Kernel Team, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus
2021-09-02 0:16 ` Saravana Kannan
@ 2021-09-02 1:43 ` Saravana Kannan
0 siblings, 0 replies; 11+ messages in thread
From: Saravana Kannan @ 2021-09-02 1:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Marek Szyprowski,
Android Kernel Team, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-02 1:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 22:45 [PATCH v1] driver core: fw_devlink: Don't create device links for devices not on a bus Saravana Kannan
2021-09-01 6:24 ` Greg Kroah-Hartman
2021-09-01 6:57 ` Saravana Kannan
2021-09-01 7:14 ` Greg Kroah-Hartman
2021-09-01 21:55 ` Saravana Kannan
2021-09-01 22:11 ` Ulf Hansson
2021-09-01 15:44 ` Ulf Hansson
2021-09-01 21:53 ` Saravana Kannan
2021-09-01 22:17 ` Ulf Hansson
2021-09-02 0:16 ` Saravana Kannan
2021-09-02 1:43 ` Saravana Kannan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.