[PATCH] driver core:Export the symbol device_is_bound

[PATCH] driver core:Export the symbol device_is_bound

[Sandeep Maheswaram]

Export the symbol device_is_bound so that it can be used by the modules.
This change was suggested to solve the allmodconfig build error on adding
the patch https://lore.kernel.org/patchwork/patch/1218628/

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 drivers/base/dd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940..65d16ce 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -337,6 +337,7 @@ bool device_is_bound(struct device *dev)
 {
 	return dev->p && klist_node_attached(&dev->p->knode_driver);
 }
+EXPORT_SYMBOL_GPL(device_is_bound);
 
 static void driver_bound(struct device *dev)
 {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] driver core:Export the symbol device_is_bound

[Matthias Kaehlcke]

On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> Export the symbol device_is_bound so that it can be used by the modules.
> This change was suggested to solve the allmodconfig build error on adding
> the patch https://lore.kernel.org/patchwork/patch/1218628/

nit: the last two lines aren't particularly interesting in the commit log,
this could be mentioned below '---'.

It might make sense to group the two patches in a series, which would make
the dependency more evident.

> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
>  drivers/base/dd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940..65d16ce 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -337,6 +337,7 @@ bool device_is_bound(struct device *dev)
>  {
>  	return dev->p && klist_node_attached(&dev->p->knode_driver);
>  }
> +EXPORT_SYMBOL_GPL(device_is_bound);
>  
>  static void driver_bound(struct device *dev)
>  {

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH] driver core:Export the symbol device_is_bound

[Greg Kroah-Hartman]

On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> Export the symbol device_is_bound so that it can be used by the modules.

What modules need this?

> This change was suggested to solve the allmodconfig build error on adding
> the patch https://lore.kernel.org/patchwork/patch/1218628/
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
>  drivers/base/dd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940..65d16ce 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -337,6 +337,7 @@ bool device_is_bound(struct device *dev)
>  {
>  	return dev->p && klist_node_attached(&dev->p->knode_driver);
>  }
> +EXPORT_SYMBOL_GPL(device_is_bound);

If a driver needs to use this, something is really wrong with it.  What
happens right after this, the state could have changed?

So, no, sorry, this is not a good idea.

thanks,

greg k-h

Re: [PATCH] driver core:Export the symbol device_is_bound

[Matthias Kaehlcke]

Hi Greg,

On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> > Export the symbol device_is_bound so that it can be used by the modules.
> 
> What modules need this?

drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers').

Short summary: QCOM dwc3 support is split in two drivers, the core dwc3
driver and the QCOM specific parts. dwc3-qcom is probed first (through
a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate()
to probe the core part. After a successful return from _populate() the
driver assumes that the core device is fully initialized. However the
latter is not correct, the driver core doesn't propagate errors from
probe() to platform_populate(). The dwc3-qcom driver would use
device_is_bound() to make sure the core device was probed successfully.

Related patches:

"usb: dwc3: qcom: Make sure core device is fully initialized before it is used"
https://lore.kernel.org/patchwork/patch/1257279/

"usb: dwc3: qcom: Add interconnect support in dwc3 driver"
https://patchwork.kernel.org/patch/11468647/

> > This change was suggested to solve the allmodconfig build error on adding
> > the patch https://lore.kernel.org/patchwork/patch/1218628/
> > 
> > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> > ---
> >  drivers/base/dd.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 9a1d940..65d16ce 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -337,6 +337,7 @@ bool device_is_bound(struct device *dev)
> >  {
> >  	return dev->p && klist_node_attached(&dev->p->knode_driver);
> >  }
> > +EXPORT_SYMBOL_GPL(device_is_bound);
> 
> If a driver needs to use this, something is really wrong with it.  What
> happens right after this, the state could have changed?
> 
> So, no, sorry, this is not a good idea.

Agreed that what some dwc3 'wrapper' drivers do is brittle, and that using
device_is_bound() is only a bandaid, that doesn't address the entire issue.

Do you have any suggestions on how this could be properly addressed?

Thanks

Matthias

Re: [PATCH] driver core:Export the symbol device_is_bound

[Greg Kroah-Hartman]

On Thu, Jun 18, 2020 at 08:45:55AM -0700, Matthias Kaehlcke wrote:
> Hi Greg,
> 
> On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> > > Export the symbol device_is_bound so that it can be used by the modules.
> > 
> > What modules need this?
> 
> drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers').

Why wasn't that said here?  No context is not good :(

> Short summary: QCOM dwc3 support is split in two drivers, the core dwc3
> driver and the QCOM specific parts. dwc3-qcom is probed first (through
> a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate()
> to probe the core part. After a successful return from _populate() the
> driver assumes that the core device is fully initialized. However the
> latter is not correct, the driver core doesn't propagate errors from
> probe() to platform_populate(). The dwc3-qcom driver would use
> device_is_bound() to make sure the core device was probed successfully.

why does the dwc3-qcom driver care?

And why is the driver split in a way that requires such "broken"
structures?  Why can't that be fixed instead?

thanks,

greg k-h

Re: [PATCH] driver core:Export the symbol device_is_bound

[Matthias Kaehlcke]

On Thu, Jun 18, 2020 at 05:58:20PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 18, 2020 at 08:45:55AM -0700, Matthias Kaehlcke wrote:
> > Hi Greg,
> > 
> > On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> > > > Export the symbol device_is_bound so that it can be used by the modules.
> > > 
> > > What modules need this?
> > 
> > drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers').
> 
> Why wasn't that said here?  No context is not good :(

Agreed, this patch should probably have been part of a series to establish
the context.

> > Short summary: QCOM dwc3 support is split in two drivers, the core dwc3
> > driver and the QCOM specific parts. dwc3-qcom is probed first (through
> > a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate()
> > to probe the core part. After a successful return from _populate() the
> > driver assumes that the core device is fully initialized. However the
> > latter is not correct, the driver core doesn't propagate errors from
> > probe() to platform_populate(). The dwc3-qcom driver would use
> > device_is_bound() to make sure the core device was probed successfully.
> 
> why does the dwc3-qcom driver care?

Currently the dwc3-qcom driver uses the core device to determine if the
controller is used in peripheral mode and it runtime resumes the XHCI
device when it sees a wakeup interrupt.

The WIP patch to add interconnect support relies on the core driver
to determine the max speed of the controller.

> And why is the driver split in a way that requires such "broken"
> structures?  Why can't that be fixed instead?

It seems determining the mode could be easily changed by getting it through
the pdev, as in st_dwc3_probe(). Not sure about the other two parts,
determining the maximum speed can involve evaluating hardware registers,
which currently are 'owned' by the core driver.

Manu or Sandeep who know the hardware and the driver better than me might
have ideas on how to improve things.

Re: [PATCH] driver core:Export the symbol device_is_bound

[Rob Herring]

On Thu, Jun 18, 2020 at 10:51 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Thu, Jun 18, 2020 at 05:58:20PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 18, 2020 at 08:45:55AM -0700, Matthias Kaehlcke wrote:
> > > Hi Greg,
> > >
> > > On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> > > > > Export the symbol device_is_bound so that it can be used by the modules.
> > > >
> > > > What modules need this?
> > >
> > > drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers').
> >
> > Why wasn't that said here?  No context is not good :(
>
> Agreed, this patch should probably have been part of a series to establish
> the context.
>
> > > Short summary: QCOM dwc3 support is split in two drivers, the core dwc3
> > > driver and the QCOM specific parts. dwc3-qcom is probed first (through
> > > a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate()
> > > to probe the core part. After a successful return from _populate() the
> > > driver assumes that the core device is fully initialized. However the
> > > latter is not correct, the driver core doesn't propagate errors from
> > > probe() to platform_populate(). The dwc3-qcom driver would use
> > > device_is_bound() to make sure the core device was probed successfully.
> >
> > why does the dwc3-qcom driver care?
>
> Currently the dwc3-qcom driver uses the core device to determine if the
> controller is used in peripheral mode and it runtime resumes the XHCI
> device when it sees a wakeup interrupt.
>
> The WIP patch to add interconnect support relies on the core driver
> to determine the max speed of the controller.
>
> > And why is the driver split in a way that requires such "broken"
> > structures?  Why can't that be fixed instead?
>
> It seems determining the mode could be easily changed by getting it through
> the pdev, as in st_dwc3_probe(). Not sure about the other two parts,
> determining the maximum speed can involve evaluating hardware registers,
> which currently are 'owned' by the core driver.
>
> Manu or Sandeep who know the hardware and the driver better than me might
> have ideas on how to improve things.

We never should have had this split either in the DT binding nor
driver(s) as if the SoC wrapper crap and licensed IP block are
independent things. The thing to do here is either make the DWC3 code
a library which drivers call (e.g. SDHCI) or add hooks into the DWC3
driver for platform specifics (e.g. Designware PCI). Neither is a
simple solution though.

Rob

Re: [PATCH] driver core:Export the symbol device_is_bound

[Matthias Kaehlcke]

On Thu, Jun 18, 2020 at 11:33:49AM -0600, Rob Herring wrote:
> On Thu, Jun 18, 2020 at 10:51 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Thu, Jun 18, 2020 at 05:58:20PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 18, 2020 at 08:45:55AM -0700, Matthias Kaehlcke wrote:
> > > > Hi Greg,
> > > >
> > > > On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> > > > > > Export the symbol device_is_bound so that it can be used by the modules.
> > > > >
> > > > > What modules need this?
> > > >
> > > > drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers').
> > >
> > > Why wasn't that said here?  No context is not good :(
> >
> > Agreed, this patch should probably have been part of a series to establish
> > the context.
> >
> > > > Short summary: QCOM dwc3 support is split in two drivers, the core dwc3
> > > > driver and the QCOM specific parts. dwc3-qcom is probed first (through
> > > > a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate()
> > > > to probe the core part. After a successful return from _populate() the
> > > > driver assumes that the core device is fully initialized. However the
> > > > latter is not correct, the driver core doesn't propagate errors from
> > > > probe() to platform_populate(). The dwc3-qcom driver would use
> > > > device_is_bound() to make sure the core device was probed successfully.
> > >
> > > why does the dwc3-qcom driver care?
> >
> > Currently the dwc3-qcom driver uses the core device to determine if the
> > controller is used in peripheral mode and it runtime resumes the XHCI
> > device when it sees a wakeup interrupt.
> >
> > The WIP patch to add interconnect support relies on the core driver
> > to determine the max speed of the controller.
> >
> > > And why is the driver split in a way that requires such "broken"
> > > structures?  Why can't that be fixed instead?
> >
> > It seems determining the mode could be easily changed by getting it through
> > the pdev, as in st_dwc3_probe(). Not sure about the other two parts,
> > determining the maximum speed can involve evaluating hardware registers,
> > which currently are 'owned' by the core driver.
> >
> > Manu or Sandeep who know the hardware and the driver better than me might
> > have ideas on how to improve things.
> 
> We never should have had this split either in the DT binding nor
> driver(s) as if the SoC wrapper crap and licensed IP block are
> independent things. The thing to do here is either make the DWC3 code
> a library which drivers call (e.g. SDHCI) or add hooks into the DWC3
> driver for platform specifics (e.g. Designware PCI). Neither is a
> simple solution though.

Sounds reasonable, but as you say it's likely not a short term
solution. If someone ever picked it up maybe they could create a
new driver (with plenty of copied code from the current driver)
and start with supporting a single platform (with multi-platform
support in the driver architecture). Other drivers could then be
migrated one by one by folks who have the hardware to test.
Duplicate code is definitely not desirable, but it's probably
more feasible than migrating all the drivers in one big bang.

I guess for now we are stuck with the race in the dwc3-qcom
driver ...