| Message ID | 20220802151404.1797-4-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
On Tue, Aug 02, 2022 at 05:13:59PM +0200, Johan Hovold wrote: > 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 when not > in host mode and there is no HCD. > > As the controller can switch role at any time when in OTG mode, there's > no quick fix to this, and since reverting would leave us with broken > suspend in host-mode (wakeup triggers immediately), keep the hack for > now and only fix the NULL-pointer dereference. > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index be2e3dd36440..b75ff40f75a2 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -301,8 +301,17 @@ static void dwc3_qcom_interconnect_exit(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; > + > + if (qcom->mode != USB_DR_MODE_HOST) > + return USB_SPEED_UNKNOWN; Couldn't instead the below block in dwc3_qcom_suspend() be conditional on the controller being in host mode? if (device_may_wakeup(qcom->dev)) { qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); dwc3_qcom_enable_interrupts(qcom); } I see, the problem is that the role switch could happen at any time as the commit message says. With this patch there is also a race though, the role switch could happen just after the check and before obtaining 'hcd'.
On Wed, Aug 03, 2022 at 12:11:41PM -0700, Matthias Kaehlcke wrote: > On Tue, Aug 02, 2022 at 05:13:59PM +0200, Johan Hovold wrote: > > 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 when not > > in host mode and there is no HCD. > > > > As the controller can switch role at any time when in OTG mode, there's > > no quick fix to this, and since reverting would leave us with broken > > suspend in host-mode (wakeup triggers immediately), keep the hack for > > now and only fix the NULL-pointer dereference. > > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index be2e3dd36440..b75ff40f75a2 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -301,8 +301,17 @@ static void dwc3_qcom_interconnect_exit(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; > > + > > + if (qcom->mode != USB_DR_MODE_HOST) > > + return USB_SPEED_UNKNOWN; > > Couldn't instead the below block in dwc3_qcom_suspend() be conditional on > the controller being in host mode? > > if (device_may_wakeup(qcom->dev)) { > qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); > dwc3_qcom_enable_interrupts(qcom); > } Yeah, the authors clearly didn't consider non-host mode when implementing this and keeping wakeups disabled in that case probably doesn't break anything that was ever working. > I see, the problem is that the role switch could happen at any time as the > commit message says. With this patch there is also a race though, the role > switch could happen just after the check and before obtaining 'hcd'. No, there's no race here as I'm checking the static configuration that comes from DT. Specifically, I'm not trying to add support for wakeup in OTG mode, but just prevent suspend from crashing. I may be possible address also the host-role in OTG mode, but that means continuing to build on this layer violation. Note that we're in the suspend callback of the parent so as long as the drivers for the descendant devices has disabled role switching at this stage during suspend, we should be good. But I'm torn about simply ripping this patch out and trying to fix it up. I want the feature, but the patch adding this clearly wasn't ready for merging. I'll take another look at this. Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index be2e3dd36440..b75ff40f75a2 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -301,8 +301,17 @@ static void dwc3_qcom_interconnect_exit(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; + + if (qcom->mode != USB_DR_MODE_HOST) + return USB_SPEED_UNKNOWN; + + /* + * FIXME: Fix this nasty layering violation which currently only + * "works" in host mode. + */ + hcd = platform_get_drvdata(dwc->xhci); /* * It is possible to query the speed of all children of
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 when not in host mode and there is no HCD. As the controller can switch role at any time when in OTG mode, there's no quick fix to this, and since reverting would leave us with broken suspend in host-mode (wakeup triggers immediately), keep the hack for now and only fix the NULL-pointer dereference. Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)