| Message ID | 20220804151001.23612-7-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
Hi Johan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master next-20220804]
[cannot apply to robh/for-next v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arc-randconfig-r002-20220804 (https://download.01.org/0day-ci/archive/20220805/202208050544.ijUhoUyB-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.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
# https://github.com/intel-lab-lkp/linux/commit/f3778ca026b16474e49c5e0188a0eb91d73eef2f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122
git checkout f3778ca026b16474e49c5e0188a0eb91d73eef2f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/usb/dwc3/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_read_usb2_speed':
>> drivers/usb/dwc3/dwc3-qcom.c:313:25: warning: variable 'hcd' set but not used [-Wunused-but-set-variable]
313 | struct usb_hcd *hcd;
| ^~~
vim +/hcd +313 drivers/usb/dwc3/dwc3-qcom.c
308
309 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
310 {
311 struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
312 struct usb_device *udev;
> 313 struct usb_hcd *hcd;
314
315 /*
316 * FIXME: Fix this layering violation.
317 */
318 hcd = platform_get_drvdata(dwc->xhci);
319
320 /*
321 * It is possible to query the speed of all children of
322 * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
323 * currently supports only 1 port per controller. So
324 * this is sufficient.
325 */
326 #ifdef CONFIG_USB
327 udev = usb_hub_find_child(hcd->self.root_hub, 1);
328 #else
329 udev = NULL;
330 #endif
331 if (!udev)
332 return USB_SPEED_UNKNOWN;
333
334 return udev->speed;
335 }
336
On Fri, Aug 05, 2022 at 05:38:30AM +0800, kernel test robot wrote: > Hi Johan, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on usb/usb-testing] > [also build test WARNING on linus/master next-20220804] > [cannot apply to robh/for-next v5.19] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122 > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > config: arc-randconfig-r002-20220804 (https://download.01.org/0day-ci/archive/20220805/202208050544.ijUhoUyB-lkp@intel.com/config) > compiler: arc-elf-gcc (GCC) 12.1.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 > # https://github.com/intel-lab-lkp/linux/commit/f3778ca026b16474e49c5e0188a0eb91d73eef2f > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122 > git checkout f3778ca026b16474e49c5e0188a0eb91d73eef2f > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/usb/dwc3/ > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_read_usb2_speed': > >> drivers/usb/dwc3/dwc3-qcom.c:313:25: warning: variable 'hcd' set but not used [-Wunused-but-set-variable] > 313 | struct usb_hcd *hcd; > | ^~~> I'm not seeing this one with gcc-10.3.0, but I'll slap a __maybe_unused in there to keep your robot's W=1 builds quiet. Johan
On Fri, Aug 05, 2022 at 08:58:00AM +0200, Johan Hovold wrote: > On Fri, Aug 05, 2022 at 05:38:30AM +0800, kernel test robot wrote: > > Hi Johan, > > > > I love your patch! Perhaps something to improve: > > > > [auto build test WARNING on usb/usb-testing] > > [also build test WARNING on linus/master next-20220804] > > [cannot apply to robh/for-next v5.19] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > > config: arc-randconfig-r002-20220804 (https://download.01.org/0day-ci/archive/20220805/202208050544.ijUhoUyB-lkp@intel.com/config) > > compiler: arc-elf-gcc (GCC) 12.1.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 > > # https://github.com/intel-lab-lkp/linux/commit/f3778ca026b16474e49c5e0188a0eb91d73eef2f > > git remote add linux-review https://github.com/intel-lab-lkp/linux > > git fetch --no-tags linux-review Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122 > > git checkout f3778ca026b16474e49c5e0188a0eb91d73eef2f > > # save the config file > > mkdir build_dir && cp config build_dir/.config > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/usb/dwc3/ > > > > If you fix the issue, kindly add following tag where applicable > > Reported-by: kernel test robot <lkp@intel.com> > > > > All warnings (new ones prefixed by >>): > > > > drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_read_usb2_speed': > > >> drivers/usb/dwc3/dwc3-qcom.c:313:25: warning: variable 'hcd' set but not used [-Wunused-but-set-variable] > > 313 | struct usb_hcd *hcd; > > | ^~~> > > I'm not seeing this one with gcc-10.3.0, but I'll slap a __maybe_unused > in there to keep your robot's W=1 builds quiet. Correction: of course I'm seeing it in the affected build configuration... Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 05b4666fde14..6ae0b7fc4e2c 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -309,8 +309,13 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) { struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); - struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci); struct usb_device *udev; + struct usb_hcd *hcd; + + /* + * FIXME: Fix this layering violation. + */ + hcd = platform_get_drvdata(dwc->xhci); /* * It is possible to query the speed of all children of @@ -416,7 +421,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) if (ret) dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret); - if (wakeup) { + /* + * The role is stable during suspend as role switching is done from a + * freezable workqueue. + */ + if (dwc3_qcom_is_host(qcom) && wakeup) { qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); dwc3_qcom_enable_interrupts(qcom); } @@ -434,7 +443,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) if (!qcom->is_suspended) return 0; - if (wakeup) + if (dwc3_qcom_is_host(qcom) && wakeup) dwc3_qcom_disable_interrupts(qcom); for (i = 0; i < qcom->num_clocks; i++) {
A recent commit implementing wakeup support in host mode instead broke suspend for peripheral and OTG mode. The hack that was added in the suspend path to determine the speed of any device connected to the USB2 bus not only accesses internal driver data for a child device, but also dereferences a NULL pointer or accesses freed data when the controller is not acting as host. There's no quick fix to the layering violation, but since reverting would leave us with broken suspend in host mode with wakeup triggering immediately, let's keep the hack for now. Fix the immediate issues by only checking the host bus speed and enabling wakeup interrupts when acting as host. Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Changes in v2 - disable wakeup completely instead of falling back to the "disconnected" host configuration when not acting as host drivers/usb/dwc3/dwc3-qcom.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)