| Message ID | 20200616133734.1.I1902d0d48e4f3d4c5b5f1a2008108a4cd3c5ddb5@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: qcom: Make sure core device is fully initialized before it is used | expand |
Quoting Matthias Kaehlcke (2020-06-16 13:37:37) > dwc3_qcom_of_register_core() uses of_platform_populate() to add > the dwc3 core device. The driver core will try to probe the device, > however this might fail (e.g. due to deferred probing) and > of_platform_populate() would still return 0 if the device was > successully added to the platform bus. Verify that the core device > is actually bound to its driver before using it, defer probing of the > dwc3_qcom device if the core device isn't ready (yet). > > Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver"). > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > depends on: > https://lore.kernel.org/patchwork/patch/1251661/ ("driver core:Export > the symbol device_is_bound") > > drivers/usb/dwc3/dwc3-qcom.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 1dfd024cd06b..5a9036b050c6 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -537,6 +537,16 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev) > return -ENODEV; > } > > + /* > + * A successful return from of_platform_populate() only guarantees that > + * the core device was added to the platform bus, however it might not > + * be bound to its driver (e.g. due to deferred probing). This driver > + * requires the core device to be fully initialized, so defer probing > + * if it isn't ready (yet). > + */ > + if (!device_is_bound(&qcom->dwc3->dev)) > + return -EPROBE_DEFER; Isn't this still broken? i.e. the dwc3 core driver may bind much later and then device_is_bound() will return an error here and then we'll return to the caller, dwc3_qcom_probe(), and that will depopulate the device with of_platform_depopulate(). It seems like we need to run some sort of wait for driver to be bound function instead of a one-shot check for the driver being bound. > + > return 0; Also, what about acpi? That has the same problem but it isn't covered by the dwc3_qcom_of_register_core() function.
Hi Stephen, On Wed, Jun 17, 2020 at 12:49:13PM -0700, Stephen Boyd wrote: > Quoting Matthias Kaehlcke (2020-06-16 13:37:37) > > dwc3_qcom_of_register_core() uses of_platform_populate() to add > > the dwc3 core device. The driver core will try to probe the device, > > however this might fail (e.g. due to deferred probing) and > > of_platform_populate() would still return 0 if the device was > > successully added to the platform bus. Verify that the core device > > is actually bound to its driver before using it, defer probing of the > > dwc3_qcom device if the core device isn't ready (yet). > > > > Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver"). > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > depends on: > > https://lore.kernel.org/patchwork/patch/1251661/ ("driver core:Export > > the symbol device_is_bound") > > > > drivers/usb/dwc3/dwc3-qcom.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index 1dfd024cd06b..5a9036b050c6 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -537,6 +537,16 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev) > > return -ENODEV; > > } > > > > + /* > > + * A successful return from of_platform_populate() only guarantees that > > + * the core device was added to the platform bus, however it might not > > + * be bound to its driver (e.g. due to deferred probing). This driver > > + * requires the core device to be fully initialized, so defer probing > > + * if it isn't ready (yet). > > + */ > > + if (!device_is_bound(&qcom->dwc3->dev)) > > + return -EPROBE_DEFER; > > Isn't this still broken? i.e. the dwc3 core driver may bind much later > and then device_is_bound() will return an error here and then we'll > return to the caller, dwc3_qcom_probe(), and that will depopulate the > device with of_platform_depopulate(). It seems like we need to run some > sort of wait for driver to be bound function instead of a one-shot check > for the driver being bound. My understanding is that the probing is done synchronously and either done, failed or deferred when returning from of_platform_populate(). Ideally we would be able to differentiate between a failure and deferral, and not defer probing in case of an error, however I'm not aware of a way to do this with the current driver framework. The call flow is: of_platform_populate of_platform_bus_create of_platform_device_create_pdata of_device_add device_add bus_probe_device device_initial_probe __device_attach __device_attach_driver driver_probe_device really_probe ->probe() > Also, what about acpi? That has the same problem but it isn't covered by > the dwc3_qcom_of_register_core() function. I wouldn't be surprised if it had the same problem. I'm not familiar with ACPI though and don't have a device for testing, hence I limited the patch to the platform device.
Hi Matthias,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on balbi-usb/testing/next]
[cannot apply to agross-msm/qcom/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Matthias-Kaehlcke/usb-dwc3-qcom-Make-sure-core-device-is-fully-initialized-before-it-is-used/20200617-043856
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: riscv-allmodconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "device_is_bound" [drivers/usb/dwc3/dwc3-qcom.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 1dfd024cd06b..5a9036b050c6 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -537,6 +537,16 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev) return -ENODEV; } + /* + * A successful return from of_platform_populate() only guarantees that + * the core device was added to the platform bus, however it might not + * be bound to its driver (e.g. due to deferred probing). This driver + * requires the core device to be fully initialized, so defer probing + * if it isn't ready (yet). + */ + if (!device_is_bound(&qcom->dwc3->dev)) + return -EPROBE_DEFER; + return 0; }
dwc3_qcom_of_register_core() uses of_platform_populate() to add the dwc3 core device. The driver core will try to probe the device, however this might fail (e.g. due to deferred probing) and of_platform_populate() would still return 0 if the device was successully added to the platform bus. Verify that the core device is actually bound to its driver before using it, defer probing of the dwc3_qcom device if the core device isn't ready (yet). Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver"). Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- depends on: https://lore.kernel.org/patchwork/patch/1251661/ ("driver core:Export the symbol device_is_bound") drivers/usb/dwc3/dwc3-qcom.c | 10 ++++++++++ 1 file changed, 10 insertions(+)