When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
is unable to find device_is_bound(). The error being:
ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
undefined!
Export the symbol so that the module finds it.
Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
Signed-off-by: Sudip Mukherjee <[email protected]>
---
resending with the Fixes: tag.
drivers/base/dd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 148e81969e04..a796a57e5efb 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
{
return dev->p && klist_node_attached(&dev->p->knode_driver);
}
+EXPORT_SYMBOL(device_is_bound);
static void driver_bound(struct device *dev)
{
--
2.11.0
On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> is unable to find device_is_bound(). The error being:
> ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> undefined!
>
> Export the symbol so that the module finds it.
>
> Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
>
> resending with the Fixes: tag.
>
> drivers/base/dd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 148e81969e04..a796a57e5efb 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
> {
> return dev->p && klist_node_attached(&dev->p->knode_driver);
> }
> +EXPORT_SYMBOL(device_is_bound);
EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
Also, wait, no, don't call this, are you sure you are calling it in a
race-free way? And what branch/tree is the above commit in?
thanks,
greg k-h
Hi Greg,
On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> > is unable to find device_is_bound(). The error being:
> > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > undefined!
> >
> > Export the symbol so that the module finds it.
> >
> > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
> > Signed-off-by: Sudip Mukherjee <[email protected]>
> > ---
> >
> > resending with the Fixes: tag.
> >
> > drivers/base/dd.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 148e81969e04..a796a57e5efb 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
> > {
> > return dev->p && klist_node_attached(&dev->p->knode_driver);
> > }
> > +EXPORT_SYMBOL(device_is_bound);
>
> EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
>
> Also, wait, no, don't call this, are you sure you are calling it in a
> race-free way? And what branch/tree is the above commit in?
I have not checked fully but since it is being called from probe() I
assume the lock will be held at that time.
This is from linux-next and the original commit is in "for-next"
branch of git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git
repo. I came across this problem while doing my daily allmodconfig
builds. The build log is at
https://travis-ci.com/github/sudipm-mukherjee/linux-test/jobs/429382451#L24431
--
Regards
Sudip
On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> Hi Greg,
>
> On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> > > is unable to find device_is_bound(). The error being:
> > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > undefined!
> > >
> > > Export the symbol so that the module finds it.
> > >
> > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
> > > Signed-off-by: Sudip Mukherjee <[email protected]>
> > > ---
> > >
> > > resending with the Fixes: tag.
> > >
> > > drivers/base/dd.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 148e81969e04..a796a57e5efb 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
> > > {
> > > return dev->p && klist_node_attached(&dev->p->knode_driver);
> > > }
> > > +EXPORT_SYMBOL(device_is_bound);
> >
> > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> >
> > Also, wait, no, don't call this, are you sure you are calling it in a
> > race-free way? And what branch/tree is the above commit in?
>
> I have not checked fully but since it is being called from probe() I
> assume the lock will be held at that time.
probe() should never call this function as it makes no sense at all at
that point in time. The driver should be fixed.
This function should not be exported, sorry.
thanks,
greg k-h
Hi Greg,
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Monday, November 9, 2020 6:37 PM
> Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to fix build
> failure
>
> On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > Hi Greg,
> >
> > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> > > > is unable to find device_is_bound(). The error being:
> > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > undefined!
> > > >
> > > > Export the symbol so that the module finds it.
> > > >
> > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > support")
> > > > Signed-off-by: Sudip Mukherjee <[email protected]>
> > > > ---
> > > >
> > > > resending with the Fixes: tag.
> > > >
> > > > drivers/base/dd.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > 148e81969e04..a796a57e5efb 100644
> > > > --- a/drivers/base/dd.c
> > > > +++ b/drivers/base/dd.c
> > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev) {
> > > > return dev->p && klist_node_attached(&dev->p->knode_driver);
> > > > }
> > > > +EXPORT_SYMBOL(device_is_bound);
> > >
> > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > >
> > > Also, wait, no, don't call this, are you sure you are calling it in
> > > a race-free way? And what branch/tree is the above commit in?
> >
> > I have not checked fully but since it is being called from probe() I
> > assume the lock will be held at that time.
>
> probe() should never call this function as it makes no sense at all at that point in
> time. The driver should be fixed.
Would you suggest if any other API we can use to allow the driver to know whether
another device has been probed?
For imx scu driver in question, it has a special requirement that it depends on scu power domain
driver. However, there're a huge number (200+) of power domains for each device clock, we can't define
them all in DT for a single clock controller node.
That's why we wanted to use device_is_bound() before to check if scu power domain is ready or not to
support defer probe.
Regards
Aisheng
>
> This function should not be exported, sorry.
>
> thanks,
>
> greg k-h
Hi Aisheng,
On Mon, Nov 9, 2020 at 10:57 AM Aisheng Dong <[email protected]> wrote:
>
> Hi Greg,
>
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Monday, November 9, 2020 6:37 PM
> > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to fix build
> > failure
> >
> > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > Hi Greg,
> > >
> > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > <[email protected]> wrote:
> > > >
> > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> > > > > is unable to find device_is_bound(). The error being:
> > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > undefined!
> > > > >
> > > > > Export the symbol so that the module finds it.
> > > > >
<snip>
> >
> > probe() should never call this function as it makes no sense at all at that point in
> > time. The driver should be fixed.
>
> Would you suggest if any other API we can use to allow the driver to know whether
> another device has been probed?
>
> For imx scu driver in question, it has a special requirement that it depends on scu power domain
> driver. However, there're a huge number (200+) of power domains for each device clock, we can't define
> them all in DT for a single clock controller node.
>
> That's why we wanted to use device_is_bound() before to check if scu power domain is ready or not to
> support defer probe.
iiuc, you are waiting for "fsl,scu-pd" to be registered.
I think you might be able to use bus_for_each_dev() to check if the
device has registered with the bus or not. It will be on the bus only
after bind was successful. The bus will be "platform_bus_type".
But I am sure Greg can give you better suggestion than this. :)
--
Regards
Sudip
> From: Sudip Mukherjee <[email protected]>
> Sent: Monday, November 9, 2020 7:19 PM
>
> Hi Aisheng,
>
> On Mon, Nov 9, 2020 at 10:57 AM Aisheng Dong <[email protected]>
> wrote:
> >
> > Hi Greg,
> >
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Monday, November 9, 2020 6:37 PM
> > > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to
> > > fix build failure
> > >
> > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > Hi Greg,
> > > >
> > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails
> > > > > > as it is unable to find device_is_bound(). The error being:
> > > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > undefined!
> > > > > >
> > > > > > Export the symbol so that the module finds it.
> > > > > >
>
> <snip>
>
> > >
> > > probe() should never call this function as it makes no sense at all
> > > at that point in time. The driver should be fixed.
> >
> > Would you suggest if any other API we can use to allow the driver to
> > know whether another device has been probed?
> >
> > For imx scu driver in question, it has a special requirement that it
> > depends on scu power domain driver. However, there're a huge number
> > (200+) of power domains for each device clock, we can't define them all in DT
> for a single clock controller node.
> >
> > That's why we wanted to use device_is_bound() before to check if scu
> > power domain is ready or not to support defer probe.
>
> iiuc, you are waiting for "fsl,scu-pd" to be registered.
> I think you might be able to use bus_for_each_dev() to check if the device has
> registered with the bus or not. It will be on the bus only after bind was
> successful. The bus will be "platform_bus_type".
> But I am sure Greg can give you better suggestion than this. :)
>
Thanks for the suggestion. Seems like a working tricky method. :-)
For a possible easier way, I wonder if we can use dev_driver_string() which is already
exported in kernel.
Waiting for Greg's suggestions on which way to go.
Regards
Aisheng
>
> --
> Regards
> Sudip
On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> Hi Greg,
>
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Monday, November 9, 2020 6:37 PM
> > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to fix build
> > failure
> >
> > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > Hi Greg,
> > >
> > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > <[email protected]> wrote:
> > > >
> > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> > > > > is unable to find device_is_bound(). The error being:
> > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > undefined!
> > > > >
> > > > > Export the symbol so that the module finds it.
> > > > >
> > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > > support")
> > > > > Signed-off-by: Sudip Mukherjee <[email protected]>
> > > > > ---
> > > > >
> > > > > resending with the Fixes: tag.
> > > > >
> > > > > drivers/base/dd.c | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > > 148e81969e04..a796a57e5efb 100644
> > > > > --- a/drivers/base/dd.c
> > > > > +++ b/drivers/base/dd.c
> > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev) {
> > > > > return dev->p && klist_node_attached(&dev->p->knode_driver);
> > > > > }
> > > > > +EXPORT_SYMBOL(device_is_bound);
> > > >
> > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > > >
> > > > Also, wait, no, don't call this, are you sure you are calling it in
> > > > a race-free way? And what branch/tree is the above commit in?
> > >
> > > I have not checked fully but since it is being called from probe() I
> > > assume the lock will be held at that time.
> >
> > probe() should never call this function as it makes no sense at all at that point in
> > time. The driver should be fixed.
>
> Would you suggest if any other API we can use to allow the driver to know whether
> another device has been probed?
There is none, sorry, as that just opens up way too many problems.
> For imx scu driver in question, it has a special requirement that it depends on scu power domain
> driver. However, there're a huge number (200+) of power domains for each device clock, we can't define
> them all in DT for a single clock controller node.
>
> That's why we wanted to use device_is_bound() before to check if scu power domain is ready or not to
> support defer probe.
Use the device link functionality for this type of thing, that is what
it was created for.
thanks,
greg k-h
On Mon, Nov 09, 2020 at 11:18:56AM +0000, Sudip Mukherjee wrote:
> Hi Aisheng,
>
> On Mon, Nov 9, 2020 at 10:57 AM Aisheng Dong <[email protected]> wrote:
> >
> > Hi Greg,
> >
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Monday, November 9, 2020 6:37 PM
> > > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to fix build
> > > failure
> > >
> > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > Hi Greg,
> > > >
> > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails as it
> > > > > > is unable to find device_is_bound(). The error being:
> > > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > undefined!
> > > > > >
> > > > > > Export the symbol so that the module finds it.
> > > > > >
>
> <snip>
>
> > >
> > > probe() should never call this function as it makes no sense at all at that point in
> > > time. The driver should be fixed.
> >
> > Would you suggest if any other API we can use to allow the driver to know whether
> > another device has been probed?
> >
> > For imx scu driver in question, it has a special requirement that it depends on scu power domain
> > driver. However, there're a huge number (200+) of power domains for each device clock, we can't define
> > them all in DT for a single clock controller node.
> >
> > That's why we wanted to use device_is_bound() before to check if scu power domain is ready or not to
> > support defer probe.
>
> iiuc, you are waiting for "fsl,scu-pd" to be registered.
> I think you might be able to use bus_for_each_dev() to check if the
> device has registered with the bus or not. It will be on the bus only
> after bind was successful. The bus will be "platform_bus_type".
No, do not do that, again, no individual driver should ever have to do
that. Think about what would be involved if _every_ driver started
doing this.
> But I am sure Greg can give you better suggestion than this. :)
device link :)
thanks,
greg k-h
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Monday, November 9, 2020 7:41 PM
>
> On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > Hi Greg,
> >
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Monday, November 9, 2020 6:37 PM
> > > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to
> > > fix build failure
> > >
> > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > Hi Greg,
> > > >
> > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails
> > > > > > as it is unable to find device_is_bound(). The error being:
> > > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > undefined!
> > > > > >
> > > > > > Export the symbol so that the module finds it.
> > > > > >
> > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > > > support")
> > > > > > Signed-off-by: Sudip Mukherjee <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > resending with the Fixes: tag.
> > > > > >
> > > > > > drivers/base/dd.c | 1 +
> > > > > > 1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > > > 148e81969e04..a796a57e5efb 100644
> > > > > > --- a/drivers/base/dd.c
> > > > > > +++ b/drivers/base/dd.c
> > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev) {
> > > > > > return dev->p &&
> > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > }
> > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > >
> > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > > > >
> > > > > Also, wait, no, don't call this, are you sure you are calling it
> > > > > in a race-free way? And what branch/tree is the above commit in?
> > > >
> > > > I have not checked fully but since it is being called from probe()
> > > > I assume the lock will be held at that time.
> > >
> > > probe() should never call this function as it makes no sense at all
> > > at that point in time. The driver should be fixed.
> >
> > Would you suggest if any other API we can use to allow the driver to
> > know whether another device has been probed?
>
> There is none, sorry, as that just opens up way too many problems.
>
> > For imx scu driver in question, it has a special requirement that it
> > depends on scu power domain driver. However, there're a huge number
> > (200+) of power domains for each device clock, we can't define them all in DT
> for a single clock controller node.
> >
> > That's why we wanted to use device_is_bound() before to check if scu
> > power domain is ready or not to support defer probe.
>
> Use the device link functionality for this type of thing, that is what it was created
> for.
>
Thanks for the suggestion. I will check it how to use.
BTW, I wonder if dev_driver_string() could be an optional solution which seems a more
simple way?
Regards
Aisheng
> thanks,
>
> greg k-h
On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Monday, November 9, 2020 7:41 PM
> >
> > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > Hi Greg,
> > >
> > > > From: Greg Kroah-Hartman <[email protected]>
> > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to
> > > > fix build failure
> > > >
> > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails
> > > > > > > as it is unable to find device_is_bound(). The error being:
> > > > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > undefined!
> > > > > > >
> > > > > > > Export the symbol so that the module finds it.
> > > > > > >
> > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > > > > support")
> > > > > > > Signed-off-by: Sudip Mukherjee <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > resending with the Fixes: tag.
> > > > > > >
> > > > > > > drivers/base/dd.c | 1 +
> > > > > > > 1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > > > > 148e81969e04..a796a57e5efb 100644
> > > > > > > --- a/drivers/base/dd.c
> > > > > > > +++ b/drivers/base/dd.c
> > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev) {
> > > > > > > return dev->p &&
> > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > }
> > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > >
> > > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > > > > >
> > > > > > Also, wait, no, don't call this, are you sure you are calling it
> > > > > > in a race-free way? And what branch/tree is the above commit in?
> > > > >
> > > > > I have not checked fully but since it is being called from probe()
> > > > > I assume the lock will be held at that time.
> > > >
> > > > probe() should never call this function as it makes no sense at all
> > > > at that point in time. The driver should be fixed.
> > >
> > > Would you suggest if any other API we can use to allow the driver to
> > > know whether another device has been probed?
> >
> > There is none, sorry, as that just opens up way too many problems.
> >
> > > For imx scu driver in question, it has a special requirement that it
> > > depends on scu power domain driver. However, there're a huge number
> > > (200+) of power domains for each device clock, we can't define them all in DT
> > for a single clock controller node.
> > >
> > > That's why we wanted to use device_is_bound() before to check if scu
> > > power domain is ready or not to support defer probe.
> >
> > Use the device link functionality for this type of thing, that is what it was created
> > for.
> >
>
> Thanks for the suggestion. I will check it how to use.
> BTW, I wonder if dev_driver_string() could be an optional solution which seems a more
> simple way?
Also, how do you really know you even have a valid pointer to that other
device structure? How are you getting access to that?
thanks,
greg k-h
On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Monday, November 9, 2020 7:41 PM
> >
> > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > Hi Greg,
> > >
> > > > From: Greg Kroah-Hartman <[email protected]>
> > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > Subject: Re: [PATCH RESEND] driver core: export device_is_bound() to
> > > > fix build failure
> > > >
> > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build fails
> > > > > > > as it is unable to find device_is_bound(). The error being:
> > > > > > > ERROR: modpost: "device_is_bound" [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > undefined!
> > > > > > >
> > > > > > > Export the symbol so that the module finds it.
> > > > > > >
> > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > > > > support")
> > > > > > > Signed-off-by: Sudip Mukherjee <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > resending with the Fixes: tag.
> > > > > > >
> > > > > > > drivers/base/dd.c | 1 +
> > > > > > > 1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > > > > 148e81969e04..a796a57e5efb 100644
> > > > > > > --- a/drivers/base/dd.c
> > > > > > > +++ b/drivers/base/dd.c
> > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev) {
> > > > > > > return dev->p &&
> > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > }
> > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > >
> > > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > > > > >
> > > > > > Also, wait, no, don't call this, are you sure you are calling it
> > > > > > in a race-free way? And what branch/tree is the above commit in?
> > > > >
> > > > > I have not checked fully but since it is being called from probe()
> > > > > I assume the lock will be held at that time.
> > > >
> > > > probe() should never call this function as it makes no sense at all
> > > > at that point in time. The driver should be fixed.
> > >
> > > Would you suggest if any other API we can use to allow the driver to
> > > know whether another device has been probed?
> >
> > There is none, sorry, as that just opens up way too many problems.
> >
> > > For imx scu driver in question, it has a special requirement that it
> > > depends on scu power domain driver. However, there're a huge number
> > > (200+) of power domains for each device clock, we can't define them all in DT
> > for a single clock controller node.
> > >
> > > That's why we wanted to use device_is_bound() before to check if scu
> > > power domain is ready or not to support defer probe.
> >
> > Use the device link functionality for this type of thing, that is what it was created
> > for.
> >
>
> Thanks for the suggestion. I will check it how to use.
> BTW, I wonder if dev_driver_string() could be an optional solution which seems a more
> simple way?
No, that's a horrible hack, do not do that, it will not work properly.
Fix this correctly.
thanks,
greg k-h
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Monday, November 9, 2020 8:05 PM
>
> On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Monday, November 9, 2020 7:41 PM
> > >
> > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > > Hi Greg,
> > > >
> > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > device_is_bound() to fix build failure
> > > > >
> > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > > > Hi Greg,
> > > > > >
> > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build
> > > > > > > > fails as it is unable to find device_is_bound(). The error being:
> > > > > > > > ERROR: modpost: "device_is_bound"
> [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > undefined!
> > > > > > > >
> > > > > > > > Export the symbol so that the module finds it.
> > > > > > > >
> > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > > > > > support")
> > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > <[email protected]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > resending with the Fixes: tag.
> > > > > > > >
> > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > > > > > 148e81969e04..a796a57e5efb 100644
> > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
> {
> > > > > > > > return dev->p &&
> > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > }
> > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > >
> > > > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > > > > > >
> > > > > > > Also, wait, no, don't call this, are you sure you are
> > > > > > > calling it in a race-free way? And what branch/tree is the above
> commit in?
> > > > > >
> > > > > > I have not checked fully but since it is being called from
> > > > > > probe() I assume the lock will be held at that time.
> > > > >
> > > > > probe() should never call this function as it makes no sense at
> > > > > all at that point in time. The driver should be fixed.
> > > >
> > > > Would you suggest if any other API we can use to allow the driver
> > > > to know whether another device has been probed?
> > >
> > > There is none, sorry, as that just opens up way too many problems.
> > >
> > > > For imx scu driver in question, it has a special requirement that
> > > > it depends on scu power domain driver. However, there're a huge
> > > > number
> > > > (200+) of power domains for each device clock, we can't define
> > > > them all in DT
> > > for a single clock controller node.
> > > >
> > > > That's why we wanted to use device_is_bound() before to check if
> > > > scu power domain is ready or not to support defer probe.
> > >
> > > Use the device link functionality for this type of thing, that is
> > > what it was created for.
> > >
> >
> > Thanks for the suggestion. I will check it how to use.
> > BTW, I wonder if dev_driver_string() could be an optional solution
> > which seems a more simple way?
>
> Also, how do you really know you even have a valid pointer to that other device
> structure? How are you getting access to that?
>
The rough idea is as follows. Not sure if those APIs are safe enough as there're
many users In kernel.
pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
pd_dev = of_find_device_by_node(pd_np);
if (!pd_dev || !dev_driver_string(&pd_dev->dev) ||
strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
of_node_put(pd_np);
return -EPROBE_DEFER;
}
const char *dev_driver_string(const struct device *dev)
{
struct device_driver *drv;
/* dev->driver can change to NULL underneath us because of unbinding,
* so be careful about accessing it. dev->bus and dev->class should
* never change once they are set, so they don't need special care.
*/
drv = READ_ONCE(dev->driver);
return drv ? drv->name :
(dev->bus ? dev->bus->name :
(dev->class ? dev->class->name : ""));
}
EXPORT_SYMBOL(dev_driver_string);
Anyway, I will continue to investigate device_link according to your suggestions.
Regards
Aisheng
> thanks,
>
> greg k-h
On Mon, Nov 09, 2020 at 12:26:55PM +0000, Aisheng Dong wrote:
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Monday, November 9, 2020 8:05 PM
> >
> > On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > > From: Greg Kroah-Hartman <[email protected]>
> > > > Sent: Monday, November 9, 2020 7:41 PM
> > > >
> > > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > > > Hi Greg,
> > > > >
> > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > > device_is_bound() to fix build failure
> > > > > >
> > > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee wrote:
> > > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build
> > > > > > > > > fails as it is unable to find device_is_bound(). The error being:
> > > > > > > > > ERROR: modpost: "device_is_bound"
> > [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > > undefined!
> > > > > > > > >
> > > > > > > > > Export the symbol so that the module finds it.
> > > > > > > > >
> > > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding
> > > > > > > > > support")
> > > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > > <[email protected]>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > resending with the Fixes: tag.
> > > > > > > > >
> > > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > > > > > > > > 148e81969e04..a796a57e5efb 100644
> > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device *dev)
> > {
> > > > > > > > > return dev->p &&
> > > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > > }
> > > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > > >
> > > > > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this file.
> > > > > > > >
> > > > > > > > Also, wait, no, don't call this, are you sure you are
> > > > > > > > calling it in a race-free way? And what branch/tree is the above
> > commit in?
> > > > > > >
> > > > > > > I have not checked fully but since it is being called from
> > > > > > > probe() I assume the lock will be held at that time.
> > > > > >
> > > > > > probe() should never call this function as it makes no sense at
> > > > > > all at that point in time. The driver should be fixed.
> > > > >
> > > > > Would you suggest if any other API we can use to allow the driver
> > > > > to know whether another device has been probed?
> > > >
> > > > There is none, sorry, as that just opens up way too many problems.
> > > >
> > > > > For imx scu driver in question, it has a special requirement that
> > > > > it depends on scu power domain driver. However, there're a huge
> > > > > number
> > > > > (200+) of power domains for each device clock, we can't define
> > > > > them all in DT
> > > > for a single clock controller node.
> > > > >
> > > > > That's why we wanted to use device_is_bound() before to check if
> > > > > scu power domain is ready or not to support defer probe.
> > > >
> > > > Use the device link functionality for this type of thing, that is
> > > > what it was created for.
> > > >
> > >
> > > Thanks for the suggestion. I will check it how to use.
> > > BTW, I wonder if dev_driver_string() could be an optional solution
> > > which seems a more simple way?
> >
> > Also, how do you really know you even have a valid pointer to that other device
> > structure? How are you getting access to that?
> >
>
> The rough idea is as follows. Not sure if those APIs are safe enough as there're
> many users In kernel.
>
> pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
> pd_dev = of_find_device_by_node(pd_np);
> if (!pd_dev || !dev_driver_string(&pd_dev->dev) ||
> strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
> of_node_put(pd_np);
> return -EPROBE_DEFER;
> }
Ick, again, no, don't do that, you can not guarantee "names" of devices
anywhere in the system, sorry.
greg k-h
Hi Greg,
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Monday, November 9, 2020 8:48 PM
>
> On Mon, Nov 09, 2020 at 12:26:55PM +0000, Aisheng Dong wrote:
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Monday, November 9, 2020 8:05 PM
> > >
> > > On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > Sent: Monday, November 9, 2020 7:41 PM
> > > > >
> > > > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > > > > Hi Greg,
> > > > > >
> > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > > > device_is_bound() to fix build failure
> > > > > > >
> > > > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee
> wrote:
> > > > > > > > Hi Greg,
> > > > > > > >
> > > > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee
> wrote:
> > > > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build
> > > > > > > > > > fails as it is unable to find device_is_bound(). The error being:
> > > > > > > > > > ERROR: modpost: "device_is_bound"
> > > [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > > > undefined!
> > > > > > > > > >
> > > > > > > > > > Export the symbol so that the module finds it.
> > > > > > > > > >
> > > > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells
> > > > > > > > > > binding
> > > > > > > > > > support")
> > > > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > > > <[email protected]>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > resending with the Fixes: tag.
> > > > > > > > > >
> > > > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > > index 148e81969e04..a796a57e5efb 100644
> > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device
> > > > > > > > > > *dev)
> > > {
> > > > > > > > > > return dev->p &&
> > > > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > > > }
> > > > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > > > >
> > > > > > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this
> file.
> > > > > > > > >
> > > > > > > > > Also, wait, no, don't call this, are you sure you are
> > > > > > > > > calling it in a race-free way? And what branch/tree is
> > > > > > > > > the above
> > > commit in?
> > > > > > > >
> > > > > > > > I have not checked fully but since it is being called from
> > > > > > > > probe() I assume the lock will be held at that time.
> > > > > > >
> > > > > > > probe() should never call this function as it makes no sense
> > > > > > > at all at that point in time. The driver should be fixed.
> > > > > >
> > > > > > Would you suggest if any other API we can use to allow the
> > > > > > driver to know whether another device has been probed?
> > > > >
> > > > > There is none, sorry, as that just opens up way too many problems.
> > > > >
> > > > > > For imx scu driver in question, it has a special requirement
> > > > > > that it depends on scu power domain driver. However, there're
> > > > > > a huge number
> > > > > > (200+) of power domains for each device clock, we can't define
> > > > > > them all in DT
> > > > > for a single clock controller node.
> > > > > >
> > > > > > That's why we wanted to use device_is_bound() before to check
> > > > > > if scu power domain is ready or not to support defer probe.
> > > > >
> > > > > Use the device link functionality for this type of thing, that
> > > > > is what it was created for.
> > > > >
> > > >
> > > > Thanks for the suggestion. I will check it how to use.
> > > > BTW, I wonder if dev_driver_string() could be an optional solution
> > > > which seems a more simple way?
> > >
> > > Also, how do you really know you even have a valid pointer to that
> > > other device structure? How are you getting access to that?
> > >
> >
> > The rough idea is as follows. Not sure if those APIs are safe enough
> > as there're many users In kernel.
> >
> > pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd"); pd_dev =
> > of_find_device_by_node(pd_np); if (!pd_dev ||
> > !dev_driver_string(&pd_dev->dev) ||
> > strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
> > of_node_put(pd_np);
> > return -EPROBE_DEFER;
> > }
>
> Ick, again, no, don't do that, you can not guarantee "names" of devices
> anywhere in the system, sorry.
I tried to understand how to use devlink for this case by diving deep into the devlink code,
however, it looked to me there're a few limitations on the current devlink usage.
We can't create driver presence dependency link in consumer's probe function
while the supplier driver is still not probed or even not created yet.
(we're the later case that supplier device scu-pd may be created after scu-clk device).
The kernel doc Documentation/driver-api/device_link.rst also mentioned this limitation and
the suggested solution is:
"The onus is thus on the consumer to check presence of the supplier after adding the link,
and defer probing on non-presence."
Then the question is , back again, , how to check the presense of other device driver if we can't
use device_is_bound() API in module?
The dev_driver_string() seems be a quick and dirty solution for the this build break issue as
the driver name is less possible to be changed and under control.
How would you suggest for current situation?
Regards
Aisheng
>
> greg k-h
On Wed, Nov 18, 2020 at 10:23:42AM +0000, Aisheng Dong wrote:
> Hi Greg,
>
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Monday, November 9, 2020 8:48 PM
> >
> > On Mon, Nov 09, 2020 at 12:26:55PM +0000, Aisheng Dong wrote:
> > > > From: Greg Kroah-Hartman <[email protected]>
> > > > Sent: Monday, November 9, 2020 8:05 PM
> > > >
> > > > On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > Sent: Monday, November 9, 2020 7:41 PM
> > > > > >
> > > > > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > > > > device_is_bound() to fix build failure
> > > > > > > >
> > > > > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip Mukherjee
> > wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > >
> > > > > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip Mukherjee
> > wrote:
> > > > > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the build
> > > > > > > > > > > fails as it is unable to find device_is_bound(). The error being:
> > > > > > > > > > > ERROR: modpost: "device_is_bound"
> > > > [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > > > > undefined!
> > > > > > > > > > >
> > > > > > > > > > > Export the symbol so that the module finds it.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells
> > > > > > > > > > > binding
> > > > > > > > > > > support")
> > > > > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > > > > <[email protected]>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > resending with the Fixes: tag.
> > > > > > > > > > >
> > > > > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > > > index 148e81969e04..a796a57e5efb 100644
> > > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct device
> > > > > > > > > > > *dev)
> > > > {
> > > > > > > > > > > return dev->p &&
> > > > > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > > > > }
> > > > > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > > > > >
> > > > > > > > > > EXPORT_SYMBOL_GPL() please, like all the other exports in this
> > file.
> > > > > > > > > >
> > > > > > > > > > Also, wait, no, don't call this, are you sure you are
> > > > > > > > > > calling it in a race-free way? And what branch/tree is
> > > > > > > > > > the above
> > > > commit in?
> > > > > > > > >
> > > > > > > > > I have not checked fully but since it is being called from
> > > > > > > > > probe() I assume the lock will be held at that time.
> > > > > > > >
> > > > > > > > probe() should never call this function as it makes no sense
> > > > > > > > at all at that point in time. The driver should be fixed.
> > > > > > >
> > > > > > > Would you suggest if any other API we can use to allow the
> > > > > > > driver to know whether another device has been probed?
> > > > > >
> > > > > > There is none, sorry, as that just opens up way too many problems.
> > > > > >
> > > > > > > For imx scu driver in question, it has a special requirement
> > > > > > > that it depends on scu power domain driver. However, there're
> > > > > > > a huge number
> > > > > > > (200+) of power domains for each device clock, we can't define
> > > > > > > them all in DT
> > > > > > for a single clock controller node.
> > > > > > >
> > > > > > > That's why we wanted to use device_is_bound() before to check
> > > > > > > if scu power domain is ready or not to support defer probe.
> > > > > >
> > > > > > Use the device link functionality for this type of thing, that
> > > > > > is what it was created for.
> > > > > >
> > > > >
> > > > > Thanks for the suggestion. I will check it how to use.
> > > > > BTW, I wonder if dev_driver_string() could be an optional solution
> > > > > which seems a more simple way?
> > > >
> > > > Also, how do you really know you even have a valid pointer to that
> > > > other device structure? How are you getting access to that?
> > > >
> > >
> > > The rough idea is as follows. Not sure if those APIs are safe enough
> > > as there're many users In kernel.
> > >
> > > pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd"); pd_dev =
> > > of_find_device_by_node(pd_np); if (!pd_dev ||
> > > !dev_driver_string(&pd_dev->dev) ||
> > > strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
> > > of_node_put(pd_np);
> > > return -EPROBE_DEFER;
> > > }
> >
> > Ick, again, no, don't do that, you can not guarantee "names" of devices
> > anywhere in the system, sorry.
>
> I tried to understand how to use devlink for this case by diving deep into the devlink code,
> however, it looked to me there're a few limitations on the current devlink usage.
Adding Saravana, who wrote that code to help explain it.
> We can't create driver presence dependency link in consumer's probe function
> while the supplier driver is still not probed or even not created yet.
> (we're the later case that supplier device scu-pd may be created after scu-clk device).
Sounds like your DT is set up backwards?
> The kernel doc Documentation/driver-api/device_link.rst also mentioned this limitation and
> the suggested solution is:
> "The onus is thus on the consumer to check presence of the supplier after adding the link,
> and defer probing on non-presence."
>
> Then the question is , back again, , how to check the presense of other device driver if we can't
> use device_is_bound() API in module?
Your driver should not care, as you can't get direct access to it, so
don't try to ask the driver core about it as that is racy and not
viable.
If your driver needs resources that it can not get at this point in
time, just return from probe with a defer error. That way it will be
called again after other drivers load.
> The dev_driver_string() seems be a quick and dirty solution for the this build break issue as
> the driver name is less possible to be changed and under control.
No, driver names are not ever guaranteed to stay the same and are not to
be used for this at all, sorry.
> How would you suggest for current situation?
defer your probe like all other drivers in this situation do it? What
makes this one driver so much different than the thousands of other ones
we currently have?
thanks,
greg k-h
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Wednesday, November 18, 2020 6:46 PM
>
> On Wed, Nov 18, 2020 at 10:23:42AM +0000, Aisheng Dong wrote:
> > Hi Greg,
> >
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Monday, November 9, 2020 8:48 PM
> > >
> > > On Mon, Nov 09, 2020 at 12:26:55PM +0000, Aisheng Dong wrote:
> > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > Sent: Monday, November 9, 2020 8:05 PM
> > > > >
> > > > > On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > Sent: Monday, November 9, 2020 7:41 PM
> > > > > > >
> > > > > > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > > > > > > Hi Greg,
> > > > > > > >
> > > > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > > > > > device_is_bound() to fix build failure
> > > > > > > > >
> > > > > > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip
> > > > > > > > > Mukherjee
> > > wrote:
> > > > > > > > > > Hi Greg,
> > > > > > > > > >
> > > > > > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip
> > > > > > > > > > > Mukherjee
> > > wrote:
> > > > > > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the
> > > > > > > > > > > > build fails as it is unable to find device_is_bound(). The error
> being:
> > > > > > > > > > > > ERROR: modpost: "device_is_bound"
> > > > > [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > > > > > undefined!
> > > > > > > > > > > >
> > > > > > > > > > > > Export the symbol so that the module finds it.
> > > > > > > > > > > >
> > > > > > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells
> > > > > > > > > > > > binding
> > > > > > > > > > > > support")
> > > > > > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > > resending with the Fixes: tag.
> > > > > > > > > > > >
> > > > > > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > > > > index 148e81969e04..a796a57e5efb 100644
> > > > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct
> > > > > > > > > > > > device
> > > > > > > > > > > > *dev)
> > > > > {
> > > > > > > > > > > > return dev->p &&
> > > > > > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > > > > > }
> > > > > > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > > > > > >
> > > > > > > > > > > EXPORT_SYMBOL_GPL() please, like all the other
> > > > > > > > > > > exports in this
> > > file.
> > > > > > > > > > >
> > > > > > > > > > > Also, wait, no, don't call this, are you sure you
> > > > > > > > > > > are calling it in a race-free way? And what
> > > > > > > > > > > branch/tree is the above
> > > > > commit in?
> > > > > > > > > >
> > > > > > > > > > I have not checked fully but since it is being called
> > > > > > > > > > from
> > > > > > > > > > probe() I assume the lock will be held at that time.
> > > > > > > > >
> > > > > > > > > probe() should never call this function as it makes no
> > > > > > > > > sense at all at that point in time. The driver should be fixed.
> > > > > > > >
> > > > > > > > Would you suggest if any other API we can use to allow the
> > > > > > > > driver to know whether another device has been probed?
> > > > > > >
> > > > > > > There is none, sorry, as that just opens up way too many problems.
> > > > > > >
> > > > > > > > For imx scu driver in question, it has a special
> > > > > > > > requirement that it depends on scu power domain driver.
> > > > > > > > However, there're a huge number
> > > > > > > > (200+) of power domains for each device clock, we can't
> > > > > > > > define them all in DT
> > > > > > > for a single clock controller node.
> > > > > > > >
> > > > > > > > That's why we wanted to use device_is_bound() before to
> > > > > > > > check if scu power domain is ready or not to support defer probe.
> > > > > > >
> > > > > > > Use the device link functionality for this type of thing,
> > > > > > > that is what it was created for.
> > > > > > >
> > > > > >
> > > > > > Thanks for the suggestion. I will check it how to use.
> > > > > > BTW, I wonder if dev_driver_string() could be an optional
> > > > > > solution which seems a more simple way?
> > > > >
> > > > > Also, how do you really know you even have a valid pointer to
> > > > > that other device structure? How are you getting access to that?
> > > > >
> > > >
> > > > The rough idea is as follows. Not sure if those APIs are safe
> > > > enough as there're many users In kernel.
> > > >
> > > > pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd"); pd_dev
> > > > = of_find_device_by_node(pd_np); if (!pd_dev ||
> > > > !dev_driver_string(&pd_dev->dev) ||
> > > > strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
> > > > of_node_put(pd_np);
> > > > return -EPROBE_DEFER;
> > > > }
> > >
> > > Ick, again, no, don't do that, you can not guarantee "names" of
> > > devices anywhere in the system, sorry.
> >
> > I tried to understand how to use devlink for this case by diving deep
> > into the devlink code, however, it looked to me there're a few limitations on
> the current devlink usage.
>
> Adding Saravana, who wrote that code to help explain it.
>
> > We can't create driver presence dependency link in consumer's probe
> > function while the supplier driver is still not probed or even not created yet.
> > (we're the later case that supplier device scu-pd may be created after scu-clk
> device).
>
> Sounds like your DT is set up backwards?
Yes, the dts is like below:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8qxp.dtsi?h=v5.10-rc4
scu {
compatible = "fsl,imx-scu";
mbox-names = "tx0",
"rx0",
"gip3";
mboxes = <&lsio_mu1 0 0
&lsio_mu1 1 0
&lsio_mu1 3 3>;
clk: clock-controller {
compatible = "fsl,imx8qxp-clk";
#clock-cells = <1>;
clocks = <&xtal32k &xtal24m>;
clock-names = "xtal_32KHz", "xtal_24Mhz";
};
pd: imx8qx-pd {
compatible = "fsl,imx8qxp-scu-pd";
#power-domain-cells = <1>;
};
...
Clk and pd devices under scu node are propagated by scu driver in DT order.
If moving pd node before clk node can also address this issue.
>
> > The kernel doc Documentation/driver-api/device_link.rst also mentioned
> > this limitation and the suggested solution is:
> > "The onus is thus on the consumer to check presence of the supplier
> > after adding the link, and defer probing on non-presence."
> >
> > Then the question is , back again, , how to check the presense of
> > other device driver if we can't use device_is_bound() API in module?
>
> Your driver should not care, as you can't get direct access to it, so don't try to
> ask the driver core about it as that is racy and not viable.
>
> If your driver needs resources that it can not get at this point in time, just return
> from probe with a defer error. That way it will be called again after other
> drivers load.
>
Referring to above dts snippet, you can see the problem here is that there're no power
domain property in clock-controller node which cause the clock driver unable to use the
Normal/standard way to acquire PD resources and return a EPROBE_DEFFER if failed like
Other resources, e.g. irq, gpio and etc.
That's the main reason we use device_is_bound() in clock driver to check if PD driver
has been probed.
And of_genpd_add_device() we used in scu clk driver does not support EPROBE_DEFER
and in order to maintainer the backwards compatibility of imx_clk_scu_alloc_dev() API,
We also can't return PROBE_DEFER. All those are special reasons which made things complicated
and un-easy to address in standard way.
Maybe the simplest solution is justing move scu pd node above scu clk node which is also make
Sense because it can save a huge number of defer probes.
Anyway, thanks for the suggestion and detailed, patient explanation.
I could cook a patch to remove device_is_bound() checking code from clk driver
and moving scu pd node in dt to address it if no strong objections.
Regards
Aisheng
> > The dev_driver_string() seems be a quick and dirty solution for the
> > this build break issue as the driver name is less possible to be changed and
> under control.
>
> No, driver names are not ever guaranteed to stay the same and are not to be
> used for this at all, sorry.
>
> > How would you suggest for current situation?
>
> defer your probe like all other drivers in this situation do it? What makes this
> one driver so much different than the thousands of other ones we currently
> have?
>
> thanks,
>
> greg k-h
On Wed, Nov 18, 2020 at 7:40 AM Aisheng Dong <[email protected]> wrote:
>
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Wednesday, November 18, 2020 6:46 PM
> >
> > On Wed, Nov 18, 2020 at 10:23:42AM +0000, Aisheng Dong wrote:
> > > Hi Greg,
> > >
> > > > From: Greg Kroah-Hartman <[email protected]>
> > > > Sent: Monday, November 9, 2020 8:48 PM
> > > >
> > > > On Mon, Nov 09, 2020 at 12:26:55PM +0000, Aisheng Dong wrote:
> > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > Sent: Monday, November 9, 2020 8:05 PM
> > > > > >
> > > > > > On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > > Sent: Monday, November 9, 2020 7:41 PM
> > > > > > > >
> > > > > > > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > >
> > > > > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > > > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > > > > > > device_is_bound() to fix build failure
> > > > > > > > > >
> > > > > > > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip
> > > > > > > > > > Mukherjee
> > > > wrote:
> > > > > > > > > > > Hi Greg,
> > > > > > > > > > >
> > > > > > > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip
> > > > > > > > > > > > Mukherjee
> > > > wrote:
> > > > > > > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm' the
> > > > > > > > > > > > > build fails as it is unable to find device_is_bound(). The error
> > being:
> > > > > > > > > > > > > ERROR: modpost: "device_is_bound"
> > > > > > [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > > > > > > undefined!
> > > > > > > > > > > > >
> > > > > > > > > > > > > Export the symbol so that the module finds it.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells
> > > > > > > > > > > > > binding
> > > > > > > > > > > > > support")
> > > > > > > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >
> > > > > > > > > > > > > resending with the Fixes: tag.
> > > > > > > > > > > > >
> > > > > > > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > > > > > > > > > > index 148e81969e04..a796a57e5efb 100644
> > > > > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > > > > @@ -353,6 +353,7 @@ bool device_is_bound(struct
> > > > > > > > > > > > > device
> > > > > > > > > > > > > *dev)
> > > > > > {
> > > > > > > > > > > > > return dev->p &&
> > > > > > > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > > > > > > }
> > > > > > > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > > > > > > >
> > > > > > > > > > > > EXPORT_SYMBOL_GPL() please, like all the other
> > > > > > > > > > > > exports in this
> > > > file.
> > > > > > > > > > > >
> > > > > > > > > > > > Also, wait, no, don't call this, are you sure you
> > > > > > > > > > > > are calling it in a race-free way? And what
> > > > > > > > > > > > branch/tree is the above
> > > > > > commit in?
> > > > > > > > > > >
> > > > > > > > > > > I have not checked fully but since it is being called
> > > > > > > > > > > from
> > > > > > > > > > > probe() I assume the lock will be held at that time.
> > > > > > > > > >
> > > > > > > > > > probe() should never call this function as it makes no
> > > > > > > > > > sense at all at that point in time. The driver should be fixed.
> > > > > > > > >
> > > > > > > > > Would you suggest if any other API we can use to allow the
> > > > > > > > > driver to know whether another device has been probed?
> > > > > > > >
> > > > > > > > There is none, sorry, as that just opens up way too many problems.
> > > > > > > >
> > > > > > > > > For imx scu driver in question, it has a special
> > > > > > > > > requirement that it depends on scu power domain driver.
> > > > > > > > > However, there're a huge number
> > > > > > > > > (200+) of power domains for each device clock, we can't
> > > > > > > > > define them all in DT
> > > > > > > > for a single clock controller node.
> > > > > > > > >
> > > > > > > > > That's why we wanted to use device_is_bound() before to
> > > > > > > > > check if scu power domain is ready or not to support defer probe.
> > > > > > > >
> > > > > > > > Use the device link functionality for this type of thing,
> > > > > > > > that is what it was created for.
> > > > > > > >
> > > > > > >
> > > > > > > Thanks for the suggestion. I will check it how to use.
> > > > > > > BTW, I wonder if dev_driver_string() could be an optional
> > > > > > > solution which seems a more simple way?
> > > > > >
> > > > > > Also, how do you really know you even have a valid pointer to
> > > > > > that other device structure? How are you getting access to that?
> > > > > >
> > > > >
> > > > > The rough idea is as follows. Not sure if those APIs are safe
> > > > > enough as there're many users In kernel.
> > > > >
> > > > > pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd"); pd_dev
> > > > > = of_find_device_by_node(pd_np); if (!pd_dev ||
> > > > > !dev_driver_string(&pd_dev->dev) ||
> > > > > strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
> > > > > of_node_put(pd_np);
> > > > > return -EPROBE_DEFER;
> > > > > }
> > > >
> > > > Ick, again, no, don't do that, you can not guarantee "names" of
> > > > devices anywhere in the system, sorry.
> > >
> > > I tried to understand how to use devlink for this case by diving deep
> > > into the devlink code, however, it looked to me there're a few limitations on
> > the current devlink usage.
> >
> > Adding Saravana, who wrote that code to help explain it.
> >
> > > We can't create driver presence dependency link in consumer's probe
> > > function while the supplier driver is still not probed or even not created yet.
> > > (we're the later case that supplier device scu-pd may be created after scu-clk
> > device).
> >
> > Sounds like your DT is set up backwards?
>
> Yes, the dts is like below:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8qxp.dtsi?h=v5.10-rc4
> scu {
> compatible = "fsl,imx-scu";
> mbox-names = "tx0",
> "rx0",
> "gip3";
> mboxes = <&lsio_mu1 0 0
> &lsio_mu1 1 0
> &lsio_mu1 3 3>;
>
> clk: clock-controller {
> compatible = "fsl,imx8qxp-clk";
> #clock-cells = <1>;
> clocks = <&xtal32k &xtal24m>;
> clock-names = "xtal_32KHz", "xtal_24Mhz";
> };
>
> pd: imx8qx-pd {
> compatible = "fsl,imx8qxp-scu-pd";
> #power-domain-cells = <1>;
> };
> ...
>
> Clk and pd devices under scu node are propagated by scu driver in DT order.
> If moving pd node before clk node can also address this issue.
I can bet the DT maintainers are going to NACK a DT node reordering.
The ordering of DT nodes should not matter for correctness.
>
> >
> > > The kernel doc Documentation/driver-api/device_link.rst also mentioned
> > > this limitation and the suggested solution is:
> > > "The onus is thus on the consumer to check presence of the supplier
> > > after adding the link, and defer probing on non-presence."
> > >
> > > Then the question is , back again, , how to check the presense of
> > > other device driver if we can't use device_is_bound() API in module?
> >
> > Your driver should not care, as you can't get direct access to it, so don't try to
> > ask the driver core about it as that is racy and not viable.
> >
> > If your driver needs resources that it can not get at this point in time, just return
> > from probe with a defer error. That way it will be called again after other
> > drivers load.
> >
>
> Referring to above dts snippet, you can see the problem here is that there're no power
> domain property in clock-controller node which cause the clock driver unable to use the
> Normal/standard way to acquire PD resources and return a EPROBE_DEFFER if failed like
_THIS_ is your main problem. If you add the power domain property,
your problem would be solved. Earlier in the thread you said:
> For imx scu driver in question, it has a special requirement that it depends on scu power
> domain driver. However, there're a huge number (200+) of power domains for each device
> clock, we can't define them all in DT for a single clock controller node.
But this just seems like an arbitrary position you are taking and then
digging yourself into a hole.
Does the clock controller actually depend on the power domain? If it
does, then list it in DT. If it doesn't and the real dependency is one
of the consumer devices that consume the clock provided by this clock
controller, then your clock controller should not be waiting/deferring
probe on this power domain. Leave that to the actual device that
depends on the power domain.
What are you even achieving by not listing the power domain in the DT
node of the clock controller if you are then going to have the clock
controller block on the power domain having probed?
> Other resources, e.g. irq, gpio and etc.
> That's the main reason we use device_is_bound() in clock driver to check if PD driver
> has been probed.
>
> And of_genpd_add_device() we used in scu clk driver does not support EPROBE_DEFER
> and in order to maintainer the backwards compatibility of imx_clk_scu_alloc_dev() API,
> We also can't return PROBE_DEFER. All those are special reasons which made things complicated
> and un-easy to address in standard way.
This seems to be talking about some out of tree code? So can't really
help you without seeing the code. And I don't understand what backward
compatibility you are trying to maintain within kernel code. That's
not something we try to or need to do. Just fix up the users.
> Maybe the simplest solution is justing move scu pd node above scu clk node which is also make
> Sense because it can save a huge number of defer probes.
This is 100% going to get rejected upstream. Even if you reorder the
DT nodes, there's no guarantee that your power domain device would get
probed before the clock driver.
Long story short, either
* Don't care about the power domain in your clock driver.
Or,
* List the power domain in the clock controller's DT node and then use
the normal APIs to get the power domain. And defer like any other
driver if you can't get the power domain.
-Saravana
> From: Saravana Kannan <[email protected]>
> Sent: Thursday, November 19, 2020 6:22 AM
>
> On Wed, Nov 18, 2020 at 7:40 AM Aisheng Dong <[email protected]>
> wrote:
> >
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Wednesday, November 18, 2020 6:46 PM
> > >
> > > On Wed, Nov 18, 2020 at 10:23:42AM +0000, Aisheng Dong wrote:
> > > > Hi Greg,
> > > >
> > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > Sent: Monday, November 9, 2020 8:48 PM
> > > > >
> > > > > On Mon, Nov 09, 2020 at 12:26:55PM +0000, Aisheng Dong wrote:
> > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > Sent: Monday, November 9, 2020 8:05 PM
> > > > > > >
> > > > > > > On Mon, Nov 09, 2020 at 11:55:46AM +0000, Aisheng Dong wrote:
> > > > > > > > > From: Greg Kroah-Hartman <[email protected]>
> > > > > > > > > Sent: Monday, November 9, 2020 7:41 PM
> > > > > > > > >
> > > > > > > > > On Mon, Nov 09, 2020 at 10:57:05AM +0000, Aisheng Dong
> wrote:
> > > > > > > > > > Hi Greg,
> > > > > > > > > >
> > > > > > > > > > > From: Greg Kroah-Hartman
> > > > > > > > > > > <[email protected]>
> > > > > > > > > > > Sent: Monday, November 9, 2020 6:37 PM
> > > > > > > > > > > Subject: Re: [PATCH RESEND] driver core: export
> > > > > > > > > > > device_is_bound() to fix build failure
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Nov 09, 2020 at 10:14:46AM +0000, Sudip
> > > > > > > > > > > Mukherjee
> > > > > wrote:
> > > > > > > > > > > > Hi Greg,
> > > > > > > > > > > >
> > > > > > > > > > > > On Sun, Nov 8, 2020 at 8:23 AM Greg Kroah-Hartman
> > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Sat, Nov 07, 2020 at 10:47:27PM +0000, Sudip
> > > > > > > > > > > > > Mukherjee
> > > > > wrote:
> > > > > > > > > > > > > > When CONFIG_MXC_CLK_SCU is configured as 'm'
> > > > > > > > > > > > > > the build fails as it is unable to find
> > > > > > > > > > > > > > device_is_bound(). The error
> > > being:
> > > > > > > > > > > > > > ERROR: modpost: "device_is_bound"
> > > > > > > [drivers/clk/imx/clk-imx-scu.ko]
> > > > > > > > > > > > > > undefined!
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Export the symbol so that the module finds it.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Fixes: 77d8f3068c63 ("clk: imx: scu: add two
> > > > > > > > > > > > > > cells binding
> > > > > > > > > > > > > > support")
> > > > > > > > > > > > > > Signed-off-by: Sudip Mukherjee
> > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > resending with the Fixes: tag.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > drivers/base/dd.c | 1 +
> > > > > > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/drivers/base/dd.c
> > > > > > > > > > > > > > b/drivers/base/dd.c index
> > > > > > > > > > > > > > 148e81969e04..a796a57e5efb 100644
> > > > > > > > > > > > > > --- a/drivers/base/dd.c
> > > > > > > > > > > > > > +++ b/drivers/base/dd.c
> > > > > > > > > > > > > > @@ -353,6 +353,7 @@ bool
> > > > > > > > > > > > > > device_is_bound(struct device
> > > > > > > > > > > > > > *dev)
> > > > > > > {
> > > > > > > > > > > > > > return dev->p &&
> > > > > > > > > > > > > > klist_node_attached(&dev->p->knode_driver);
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > +EXPORT_SYMBOL(device_is_bound);
> > > > > > > > > > > > >
> > > > > > > > > > > > > EXPORT_SYMBOL_GPL() please, like all the other
> > > > > > > > > > > > > exports in this
> > > > > file.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Also, wait, no, don't call this, are you sure
> > > > > > > > > > > > > you are calling it in a race-free way? And what
> > > > > > > > > > > > > branch/tree is the above
> > > > > > > commit in?
> > > > > > > > > > > >
> > > > > > > > > > > > I have not checked fully but since it is being
> > > > > > > > > > > > called from
> > > > > > > > > > > > probe() I assume the lock will be held at that time.
> > > > > > > > > > >
> > > > > > > > > > > probe() should never call this function as it makes
> > > > > > > > > > > no sense at all at that point in time. The driver should be
> fixed.
> > > > > > > > > >
> > > > > > > > > > Would you suggest if any other API we can use to allow
> > > > > > > > > > the driver to know whether another device has been probed?
> > > > > > > > >
> > > > > > > > > There is none, sorry, as that just opens up way too many
> problems.
> > > > > > > > >
> > > > > > > > > > For imx scu driver in question, it has a special
> > > > > > > > > > requirement that it depends on scu power domain driver.
> > > > > > > > > > However, there're a huge number
> > > > > > > > > > (200+) of power domains for each device clock, we
> > > > > > > > > > can't define them all in DT
> > > > > > > > > for a single clock controller node.
> > > > > > > > > >
> > > > > > > > > > That's why we wanted to use device_is_bound() before
> > > > > > > > > > to check if scu power domain is ready or not to support defer
> probe.
> > > > > > > > >
> > > > > > > > > Use the device link functionality for this type of
> > > > > > > > > thing, that is what it was created for.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Thanks for the suggestion. I will check it how to use.
> > > > > > > > BTW, I wonder if dev_driver_string() could be an optional
> > > > > > > > solution which seems a more simple way?
> > > > > > >
> > > > > > > Also, how do you really know you even have a valid pointer
> > > > > > > to that other device structure? How are you getting access to that?
> > > > > > >
> > > > > >
> > > > > > The rough idea is as follows. Not sure if those APIs are safe
> > > > > > enough as there're many users In kernel.
> > > > > >
> > > > > > pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
> > > > > > pd_dev = of_find_device_by_node(pd_np); if (!pd_dev ||
> > > > > > !dev_driver_string(&pd_dev->dev) ||
> > > > > > strcmp(dev_driver_string(&pd_dev->dev), "imx-scu-pd")) {
> > > > > > of_node_put(pd_np);
> > > > > > return -EPROBE_DEFER;
> > > > > > }
> > > > >
> > > > > Ick, again, no, don't do that, you can not guarantee "names" of
> > > > > devices anywhere in the system, sorry.
> > > >
> > > > I tried to understand how to use devlink for this case by diving
> > > > deep into the devlink code, however, it looked to me there're a
> > > > few limitations on
> > > the current devlink usage.
> > >
> > > Adding Saravana, who wrote that code to help explain it.
> > >
> > > > We can't create driver presence dependency link in consumer's
> > > > probe function while the supplier driver is still not probed or even not
> created yet.
> > > > (we're the later case that supplier device scu-pd may be created
> > > > after scu-clk
> > > device).
> > >
> > > Sounds like your DT is set up backwards?
> >
> > Yes, the dts is like below:
> >
> > scu {
> > compatible = "fsl,imx-scu";
> > mbox-names = "tx0",
> > "rx0",
> > "gip3";
> > mboxes = <&lsio_mu1 0 0
> > &lsio_mu1 1 0
> > &lsio_mu1 3 3>;
> >
> > clk: clock-controller {
> > compatible = "fsl,imx8qxp-clk";
> > #clock-cells = <1>;
> > clocks = <&xtal32k &xtal24m>;
> > clock-names = "xtal_32KHz", "xtal_24Mhz";
> > };
> >
> > pd: imx8qx-pd {
> > compatible = "fsl,imx8qxp-scu-pd";
> > #power-domain-cells = <1>;
> > };
> > ...
> >
> > Clk and pd devices under scu node are propagated by scu driver in DT order.
> > If moving pd node before clk node can also address this issue.
>
> I can bet the DT maintainers are going to NACK a DT node reordering.
> The ordering of DT nodes should not matter for correctness.
>
> >
> > >
> > > > The kernel doc Documentation/driver-api/device_link.rst also
> > > > mentioned this limitation and the suggested solution is:
> > > > "The onus is thus on the consumer to check presence of the
> > > > supplier after adding the link, and defer probing on non-presence."
> > > >
> > > > Then the question is , back again, , how to check the presense of
> > > > other device driver if we can't use device_is_bound() API in module?
> > >
> > > Your driver should not care, as you can't get direct access to it,
> > > so don't try to ask the driver core about it as that is racy and not viable.
> > >
> > > If your driver needs resources that it can not get at this point in
> > > time, just return from probe with a defer error. That way it will
> > > be called again after other drivers load.
> > >
> >
> > Referring to above dts snippet, you can see the problem here is that
> > there're no power domain property in clock-controller node which cause
> > the clock driver unable to use the Normal/standard way to acquire PD
> > resources and return a EPROBE_DEFFER if failed like
>
> _THIS_ is your main problem. If you add the power domain property, your
> problem would be solved. Earlier in the thread you said:
>
> > For imx scu driver in question, it has a special requirement that it
> > depends on scu power domain driver. However, there're a huge number
> > (200+) of power domains for each device clock, we can't define them all in DT
> for a single clock controller node.
>
> But this just seems like an arbitrary position you are taking and then digging
> yourself into a hole.
>
> Does the clock controller actually depend on the power domain? If it does, then
> list it in DT. If it doesn't and the real dependency is one of the consumer devices
> that consume the clock provided by this clock controller, then your clock
> controller should not be waiting/deferring probe on this power domain. Leave
> that to the actual device that depends on the power domain.
It's not that case.
The clocks provided by clock provider depends on a big number of various power domains.
More strictly, the clock-controller node itself (clock protocol) does not depends on power domain.
But the sub clocks created by clock protocol driver depends on each resource power domain.
See: imx_clk_scu_alloc_dev()
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/imx/clk-scu.c
That's why we didn't list power domains in clock-controller node.
>
> What are you even achieving by not listing the power domain in the DT node of
> the clock controller if you are then going to have the clock controller block on
> the power domain having probed?
>
> > Other resources, e.g. irq, gpio and etc.
> > That's the main reason we use device_is_bound() in clock driver to
> > check if PD driver has been probed.
> >
> > And of_genpd_add_device() we used in scu clk driver does not support
> > EPROBE_DEFER and in order to maintainer the backwards compatibility of
> > imx_clk_scu_alloc_dev() API, We also can't return PROBE_DEFER. All
> > those are special reasons which made things complicated and un-easy to
> address in standard way.
>
> This seems to be talking about some out of tree code? So can't really help you
> without seeing the code. And I don't understand what backward compatibility
> you are trying to maintain within kernel code. That's not something we try to or
> need to do. Just fix up the users.
Pls refer to above link for the code.
>
> > Maybe the simplest solution is justing move scu pd node above scu clk
> > node which is also make Sense because it can save a huge number of defer
> probes.
>
> This is 100% going to get rejected upstream. Even if you reorder the DT nodes,
> there's no guarantee that your power domain device would get probed before
> the clock driver.
>
The point here is saving a huge number of defer probes.
We could find a way to ensure power domain driver get probed before clock.
> Long story short, either
>
> * Don't care about the power domain in your clock driver.
>
> Or,
>
> * List the power domain in the clock controller's DT node and then use the
> normal APIs to get the power domain. And defer like any other driver if you
> can't get the power domain.
>
Yes, I understand those are for normal cases. But our case is a bit different and we don't want
imx_clk_scu() API return PROBE_DEFER which is unnecessary for a hundred of clocks.
Even we want to defer probe, we prefer to defer in imx_clk_scu_init() rather than in imx_clk_scu().
Maybe the things can be simplified as a simple requirement:
How users can make Driver A (CLK) to be probed after Driver B (PD) without explicit
firmware function dependency description (e.g. phandle in DT)?
As kernel core does not want to support it, then the left way may be change scu-pd driver
Inicall level or provide a private callback to query the readiness.
Regards
Aisheng
> -Saravana
On Thu, Nov 19, 2020 at 04:13:34AM +0000, Aisheng Dong wrote:
> > Long story short, either
> >
> > * Don't care about the power domain in your clock driver.
> >
> > Or,
> >
> > * List the power domain in the clock controller's DT node and then use the
> > normal APIs to get the power domain. And defer like any other driver if you
> > can't get the power domain.
> >
>
> Yes, I understand those are for normal cases. But our case is a bit different and we don't want
> imx_clk_scu() API return PROBE_DEFER which is unnecessary for a hundred of clocks.
> Even we want to defer probe, we prefer to defer in imx_clk_scu_init() rather than in imx_clk_scu().
What is wrong with PROBE_DEFER, that is what it is there for.
> Maybe the things can be simplified as a simple requirement:
> How users can make Driver A (CLK) to be probed after Driver B (PD) without explicit
> firmware function dependency description (e.g. phandle in DT)?
>
> As kernel core does not want to support it, then the left way may be change scu-pd driver
> Inicall level or provide a private callback to query the readiness.
No, do not mess with that, as it totally breaks when things are built as
a module.
Rely on the deferred probing, that is what it was designed for, and it
should not take very long at all, and it keeps you from having to dig
into the driver core in ways that no one else has to.
Please just fix up your DT file (sorry I didn't know ordering wouldn't
solve this), and you should be fine.
thanks,
greg k-h
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Thursday, November 19, 2020 9:10 PM
>
> On Thu, Nov 19, 2020 at 04:13:34AM +0000, Aisheng Dong wrote:
> > > Long story short, either
> > >
> > > * Don't care about the power domain in your clock driver.
> > >
> > > Or,
> > >
> > > * List the power domain in the clock controller's DT node and then
> > > use the normal APIs to get the power domain. And defer like any
> > > other driver if you can't get the power domain.
> > >
> >
> > Yes, I understand those are for normal cases. But our case is a bit
> > different and we don't want
> > imx_clk_scu() API return PROBE_DEFER which is unnecessary for a hundred of
> clocks.
> > Even we want to defer probe, we prefer to defer in imx_clk_scu_init() rather
> than in imx_clk_scu().
>
> What is wrong with PROBE_DEFER, that is what it is there for.
Yes, we can use PROBE_DEFER, just not want to defer in imx_clk_scu_init() when creating
sub clock devices. Instead, we want to defer at the beginning of clock driver probe which
can save tens of defer probes due to the same reasons that PD driver is not ready.
>
> > Maybe the things can be simplified as a simple requirement:
> > How users can make Driver A (CLK) to be probed after Driver B (PD)
> > without explicit firmware function dependency description (e.g. phandle in
> DT)?
> >
> > As kernel core does not want to support it, then the left way may be
> > change scu-pd driver Inicall level or provide a private callback to query the
> readiness.
>
> No, do not mess with that, as it totally breaks when things are built as a module.
>
Can't this be addressed by proper module dependency? E.g clock module depends
on power domain module. Then clock driver can only be loaded after power domain.
> Rely on the deferred probing, that is what it was designed for, and it should not
> take very long at all, and it keeps you from having to dig into the driver core in
> ways that no one else has to.
>
> Please just fix up your DT file (sorry I didn't know ordering wouldn't solve this),
> and you should be fine.
>
Never mind and thank you for bringing me to the new features of fw_devlink which
I believe is good and can help us a lot on boot optimization and driver probes.
We may enable it in the future by default.
For the current problem, the difficulty is i don't know how to do it in standard way
as we can't add power domains property in clock-controller node.
The reason is described here that clock-controller (SCU clock protocol) itself does not
Depends on power domain.
https://www.spinics.net/lists/kernel/msg3742050.html
scu {
compatible = "fsl,imx-scu";
...
clk: clock-controller {
compatible = "fsl,imx8qxp-clk";
#clock-cells = <2>;
clocks = <&xtal32k &xtal24m>;
clock-names = "xtal_32KHz", "xtal_24Mhz";
};
pd: imx8qx-pd {
compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";
#power-domain-cells = <1>;
};
...
}
Regards
Aisheng
> thanks,
>
> greg k-h
On Thu, Nov 19, 2020 at 02:09:42PM +0000, Aisheng Dong wrote:
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Thursday, November 19, 2020 9:10 PM
> >
> > On Thu, Nov 19, 2020 at 04:13:34AM +0000, Aisheng Dong wrote:
> > > > Long story short, either
> > > >
> > > > * Don't care about the power domain in your clock driver.
> > > >
> > > > Or,
> > > >
> > > > * List the power domain in the clock controller's DT node and then
> > > > use the normal APIs to get the power domain. And defer like any
> > > > other driver if you can't get the power domain.
> > > >
> > >
> > > Yes, I understand those are for normal cases. But our case is a bit
> > > different and we don't want
> > > imx_clk_scu() API return PROBE_DEFER which is unnecessary for a hundred of
> > clocks.
> > > Even we want to defer probe, we prefer to defer in imx_clk_scu_init() rather
> > than in imx_clk_scu().
> >
> > What is wrong with PROBE_DEFER, that is what it is there for.
>
> Yes, we can use PROBE_DEFER, just not want to defer in imx_clk_scu_init() when creating
> sub clock devices. Instead, we want to defer at the beginning of clock driver probe which
> can save tens of defer probes due to the same reasons that PD driver is not ready.
There's nothing wrong with deferring that many times until your proper
driver is loaded, what does it cost you to do so?
> > > Maybe the things can be simplified as a simple requirement:
> > > How users can make Driver A (CLK) to be probed after Driver B (PD)
> > > without explicit firmware function dependency description (e.g. phandle in
> > DT)?
> > >
> > > As kernel core does not want to support it, then the left way may be
> > > change scu-pd driver Inicall level or provide a private callback to query the
> > readiness.
> >
> > No, do not mess with that, as it totally breaks when things are built as a module.
> >
>
> Can't this be addressed by proper module dependency? E.g clock module depends
> on power domain module. Then clock driver can only be loaded after power domain.
Sure, if you can do that, make your modules load properly by symbol
dependency and all should be fine. Have you done that?
thanks,
greg k-h
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Thursday, November 19, 2020 10:26 PM
>
> On Thu, Nov 19, 2020 at 02:09:42PM +0000, Aisheng Dong wrote:
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Thursday, November 19, 2020 9:10 PM
> > >
> > > On Thu, Nov 19, 2020 at 04:13:34AM +0000, Aisheng Dong wrote:
> > > > > Long story short, either
> > > > >
> > > > > * Don't care about the power domain in your clock driver.
> > > > >
> > > > > Or,
> > > > >
> > > > > * List the power domain in the clock controller's DT node and
> > > > > then use the normal APIs to get the power domain. And defer like
> > > > > any other driver if you can't get the power domain.
> > > > >
> > > >
> > > > Yes, I understand those are for normal cases. But our case is a
> > > > bit different and we don't want
> > > > imx_clk_scu() API return PROBE_DEFER which is unnecessary for a
> > > > hundred of
> > > clocks.
> > > > Even we want to defer probe, we prefer to defer in
> > > > imx_clk_scu_init() rather
> > > than in imx_clk_scu().
> > >
> > > What is wrong with PROBE_DEFER, that is what it is there for.
> >
> > Yes, we can use PROBE_DEFER, just not want to defer in
> > imx_clk_scu_init() when creating sub clock devices. Instead, we want
> > to defer at the beginning of clock driver probe which can save tens of defer
> probes due to the same reasons that PD driver is not ready.
>
> There's nothing wrong with deferring that many times until your proper driver is
> loaded, what does it cost you to do so?
One problem is that current imx8qxp-clk driver allows sub clocks register optionally fail.
That's also the reason we don't want defer probe during register sub clocks.
static int imx8qxp_clk_probe(struct platform_device *pdev)
{
clks[IMX_LSIO_PWM0_CLK] = imx_clk_scu("pwm0_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER, clk_cells);
clks[IMX_LSIO_PWM1_CLK] = imx_clk_scu("pwm1_clk", IMX_SC_R_PWM_1, IMX_SC_PM_CLK_PER, clk_cells)
....
return of_clk_add_hw_provider(ccm_node, imx_scu_of_clk_src_get, imx_scu_clks);
}
>
> > > > Maybe the things can be simplified as a simple requirement:
> > > > How users can make Driver A (CLK) to be probed after Driver B (PD)
> > > > without explicit firmware function dependency description (e.g.
> > > > phandle in
> > > DT)?
> > > >
> > > > As kernel core does not want to support it, then the left way may
> > > > be change scu-pd driver Inicall level or provide a private
> > > > callback to query the
> > > readiness.
> > >
> > > No, do not mess with that, as it totally breaks when things are built as a
> module.
> > >
> >
> > Can't this be addressed by proper module dependency? E.g clock module
> > depends on power domain module. Then clock driver can only be loaded after
> power domain.
>
> Sure, if you can do that, make your modules load properly by symbol
> dependency and all should be fine. Have you done that?
Still no. I planned to do that in another separate patch
The rough idea may be:
In PD driver, export an API like:
Bool imx_scu_pd_is_initilized(void);
In SCU clock driver:
If (!imx_scu_pd_is_initialized())
return -EPROBE_DEFER;
BTW I've already sent out a patch to remove the calling of device_is_bound() in order to fix
the build break first.
https://patchwork.kernel.org/project/linux-clk/patch/[email protected]/
It's okay to remove it first as DT patch using this code still not merged in mainline.
Regards
Aisheng
>
> thanks,
>
> greg k-h