Date:   Wed, 18 Nov 2020 10:23:42 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>
Subject: RE: [PATCH RESEND] driver core: export device_is_bound() to fix build
 failure

Hi Greg,

> From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> 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 <gregkh@...uxfoundation.org>
> > > 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 <gregkh@...uxfoundation.org>
> > > > > 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 <gregkh@...uxfoundation.org>
> > > > > > > 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
> > > > > > > > <gregkh@...uxfoundation.org> 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
> > > > > > > > > > <sudipm.mukherjee@...il.com>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > 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

Powered by blists - more mailing lists