mbox series

[v2,0/9] usb: dwc3: qcom: fix wakeup implementation

Message ID 20220804151001.23612-1-johan+linaro@kernel.org
Headers show
Series usb: dwc3: qcom: fix wakeup implementation | expand

Message

Johan Hovold Aug. 4, 2022, 3:09 p.m. UTC
This series fixes some of the fallout after the recently merged series
that added wakeup support to the Qualcomm dwc3 driver:

	https://lore.kernel.org/all/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/

The first patch fixes a long standing PHY power sequencing issue in dwc3
core.

The second patch reverts a power-domain hack that was added by the above
series. There are other ways to implement this which doesn't violate the
genpd interface, and if for some reason those are not sufficient, the
genpd implementation needs to be extended, not hacked around.

The third patch fixes a build breakage caused by the same series. (new
in v2)

The fourth patch fixes another long-standing bug which could lead to a
use-after-free when using runtime PM. (new in v2)

The next patch fixes another issue in the Qualcomm dwc3 implementation
that has been added a while back and which breaks runtime PM.

The sixt patch fixes a NULL-pointer dereference or use-after-free when
suspending controllers in peripheral or OTG mode due to a hack that was
added to suspend path. Unfortunately, it seems the hack needs to stay
for now if we want functioning suspend on some Qualcomm platforms.

The remaining patches moves the wakeup-source property over from the
core node to the glue node in the binding and instead propagates the
wakeup capability to the former during probe.

Note that this incidentally also avoids adding probe-deferral hacks to
the driver as was recently proposed to deal with another problem with
the current implementation:

	https://lore.kernel.org/all/1657891312-21748-1-git-send-email-quic_kriskura@quicinc.com/

With this series I have functioning USB system suspend and wakeup as
well as somewhat functioning runtime PM in host mode on sc8280xp. Note
that the PHYs apparently do not not to be left enabled for wakeup on
this platform.

Some issues remain such as that root-hub connect/disconnect events
cannot selectively be disabled.

And of course, the suspend speed hack needs to be replaced at some
point but that likely requires some more heavy lifting in the dwc3
implementation.

Johan

Changes in v2
 - add review and ack tags
 - fix a gadget-only build breakage (new patch)
 - fix a use-after-free on wakeup from runtime suspend (new patch)
 - disable wakeup completely instead of falling back to the
   "disconnected" host configuration when not acting as host
 - disallow 'wakeup-source' in child node in the binding


Johan Hovold (9):
  usb: dwc3: fix PHY disable sequence
  Revert "usb: dwc3: qcom: Keep power domain on to retain controller
    status"
  usb: dwc3: qcom: fix gadget-only builds
  usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup
  usb: dwc3: qcom: fix runtime PM wakeup
  usb: dwc3: qcom: fix peripheral and OTG suspend
  dt-bindings: usb: qcom,dwc3: add wakeup-source property
  usb: dwc3: qcom: fix wakeup implementation
  usb: dwc3: qcom: clean up suspend callbacks

 .../devicetree/bindings/usb/qcom,dwc3.yaml    |  5 +
 drivers/usb/dwc3/core.c                       | 24 ++---
 drivers/usb/dwc3/dwc3-qcom.c                  | 96 +++++++++++--------
 drivers/usb/dwc3/host.c                       |  1 +
 4 files changed, 76 insertions(+), 50 deletions(-)

Comments

Manivannan Sadhasivam Aug. 6, 2022, 2:15 p.m. UTC | #1
On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> A recent change added a dependency to the USB host stack and broke
> gadget-only builds of the driver.
> 
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> --`-
> 
> Changes in v2
>  - new patch
> 
>  drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index be2e3dd36440..e9364141661b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
>  	 * currently supports only 1 port per controller. So
>  	 * this is sufficient.
>  	 */
> +#ifdef CONFIG_USB
>  	udev = usb_hub_find_child(hcd->self.root_hub, 1);
> -
> +#else
> +	udev = NULL;
> +#endif

Perhaps the check should be moved to the caller instead? This function still
references "usb_hcd" struct and I don't think that's intended for gadget only
mode.

Thanks,
Mani

>  	if (!udev)
>  		return USB_SPEED_UNKNOWN;
>  
> -- 
> 2.35.1
>
Manivannan Sadhasivam Aug. 6, 2022, 2:35 p.m. UTC | #2
On Thu, Aug 04, 2022 at 05:09:57PM +0200, Johan Hovold wrote:
> A device must enable wakeups during runtime suspend regardless of
> whether it is capable and allowed to wake the system up from system
> suspend.
> 
> Fixes: 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6884026b9fad..05b4666fde14 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -397,7 +397,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>  	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
>  }
>  
> -static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> +static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  {
>  	u32 val;
>  	int i, ret;
> @@ -416,7 +416,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>  	if (ret)
>  		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>  
> -	if (device_may_wakeup(qcom->dev)) {
> +	if (wakeup) {
>  		qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
>  		dwc3_qcom_enable_interrupts(qcom);
>  	}
> @@ -426,7 +426,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>  	return 0;
>  }
>  
> -static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> +static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>  {
>  	int ret;
>  	int i;
> @@ -434,7 +434,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>  	if (!qcom->is_suspended)
>  		return 0;
>  
> -	if (device_may_wakeup(qcom->dev))
> +	if (wakeup)
>  		dwc3_qcom_disable_interrupts(qcom);
>  
>  	for (i = 0; i < qcom->num_clocks; i++) {
> @@ -945,9 +945,11 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
>  static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
>  {
>  	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> +	bool wakeup = device_may_wakeup(dev);
>  	int ret = 0;
>  
> -	ret = dwc3_qcom_suspend(qcom);
> +
> +	ret = dwc3_qcom_suspend(qcom, wakeup);
>  	if (!ret)
>  		qcom->pm_suspended = true;
>  
> @@ -957,9 +959,10 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
>  static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
>  {
>  	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> +	bool wakeup = device_may_wakeup(dev);
>  	int ret;
>  
> -	ret = dwc3_qcom_resume(qcom);
> +	ret = dwc3_qcom_resume(qcom, wakeup);
>  	if (!ret)
>  		qcom->pm_suspended = false;
>  
> @@ -970,14 +973,14 @@ static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
>  {
>  	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
>  
> -	return dwc3_qcom_suspend(qcom);
> +	return dwc3_qcom_suspend(qcom, true);
>  }
>  
>  static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
>  {
>  	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
>  
> -	return dwc3_qcom_resume(qcom);
> +	return dwc3_qcom_resume(qcom, true);
>  }
>  
>  static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
> -- 
> 2.35.1
>
Johan Hovold Aug. 6, 2022, 4:08 p.m. UTC | #3
On Sat, Aug 06, 2022 at 08:03:11PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:09:56PM +0200, Johan Hovold wrote:
> > The Qualcomm dwc3 runtime-PM implementation checks the xhci
> > platform-device pointer in the wakeup-interrupt handler to determine
> > whether the controller is in host mode and if so triggers a resume.
> > 
> > After a role switch in OTG mode the xhci platform-device would have been
> > freed and the next wakeup from runtime suspend would access the freed
> > memory.
> > 
> > Note that role switching is executed from a freezable workqueue, which
> > guarantees that the pointer is stable during suspend.
> > 
> > Also note that runtime PM has been broken since commit 2664deb09306
> > ("usb: dwc3: qcom: Honor wakeup enabled/disabled state"), which
> > incidentally also prevents this issue from being triggered.
> > 
> > Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")
> > Cc: stable@vger.kernel.org      # 4.18
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> It'd be good to mention the introduction of dwc3_qcom_is_host() function.
> Initially I thought it is used in a single place, but going through the rest of
> the patches reveals that it is used later on.

I think the helper is warranted on its own as it serves as documentation
of the underlying assumptions that this code relies on.

> > +/* Only usable in contexts where the role can not change. */
> > +static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
> > +{
> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > +
> > +	return dwc->xhci;
> > +}
> > +
> >  static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> >  {
> >  	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > @@ -460,7 +468,11 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> >  	if (qcom->pm_suspended)
> >  		return IRQ_HANDLED;
> >  
> > -	if (dwc->xhci)
> > +	/*
> > +	 * This is safe as role switching is done from a freezable workqueue
> > +	 * and the wakeup interrupts are disabled as part of resume.
> > +	 */
> > +	if (dwc3_qcom_is_host(qcom))
> >  		pm_runtime_resume(&dwc->xhci->dev);
> >  
> >  	return IRQ_HANDLED;
> > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > index f56c30cf151e..f6f13e7f1ba1 100644
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -135,4 +135,5 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  void dwc3_host_exit(struct dwc3 *dwc)
> >  {
> >  	platform_device_unregister(dwc->xhci);
> > +	dwc->xhci = NULL;
> >  }
> > -- 
> > 2.35.1

Johan
Johan Hovold Aug. 6, 2022, 4:33 p.m. UTC | #4
On Sat, Aug 06, 2022 at 08:27:19PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > It is the Qualcomm glue wakeup interrupts that may be able to wake the
> > system from suspend and this can now be described in the devicetree.
> > 
> > Move the wakeup-source property handling over from the core driver and
> > instead propagate the capability setting to the core device during
> > probe.

> The "wakeup-source" property is still defined in the DWC binding, so other
> platform glue drivers are free to assume that wakeup capability is handled by
> the DWC driver.

No, just because the binding says that the hardware supports something
doesn't mean it's implemented. And in this case it isn't.

There's no core support for wakeup and this is just used to determine
the PHY power state during suspend (see my reply to Matthias).

But this is also why I initially suggested reverting the binding change
until some platform actually turns out to need it.
 
> > This is needed as there is currently no way for the core driver to query
> > the wakeup setting of the glue device, but it is the core driver that
> > manages the PHY power state during suspend.
> > 
> > Also don't leave the PHYs enabled when system wakeup has been disabled
> > through sysfs.
> > 
> 
> Can you please elaborate on how this is handled in the patch?

Generally device_can_wakeup() should not be used to make policy
decisions (e.g. whether to power of the PHYs) as this should be
configurable through sysfs which is honoured by device_may_wakeup().

But I'll revisit this in a couple of weeks. We should probably just
revert the patch that made the PHY power state depend on
device_can_wakeup() as it apparently isn't needed for wakeup at all.

> > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/usb/dwc3/core.c      | 5 ++---
> >  drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 16d1f328775f..8c8e32651473 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, dwc);
> >  	dwc3_cache_hwparams(dwc);
> > -	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> >  
> >  	spin_lock_init(&dwc->lock);
> >  	mutex_init(&dwc->mutex);
> > @@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >  		dwc3_core_exit(dwc);
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> > -		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
> > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> >  			dwc3_core_exit(dwc);
> >  			break;
> >  		}
> > @@ -2045,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> >  		spin_unlock_irqrestore(&dwc->lock, flags);
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> > -		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
> > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> >  			ret = dwc3_core_init_for_resume(dwc);
> >  			if (ret)
> >  				return ret;
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 6ae0b7fc4e2c..b05f67d206d2 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  	struct resource		*res, *parent_res = NULL;
> >  	int			ret, i;
> >  	bool			ignore_pipe_clk;
> > +	bool			wakeup_source;
> >  
> >  	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
> >  	if (!qcom)
> > @@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto interconnect_exit;
> >  
> > -	device_init_wakeup(&pdev->dev, 1);
> > +	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> > +	device_init_wakeup(&pdev->dev, wakeup_source);
> > +	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> > +
> >  	qcom->is_suspended = false;
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> > -- 
> > 2.35.1

Johan
Manivannan Sadhasivam Aug. 6, 2022, 4:42 p.m. UTC | #5
On Sat, Aug 06, 2022 at 06:04:21PM +0200, Johan Hovold wrote:
> On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > > A recent change added a dependency to the USB host stack and broke
> > > gadget-only builds of the driver.
> > > 
> > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > --`-
> > > 
> > > Changes in v2
> > >  - new patch
> > > 
> > >  drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index be2e3dd36440..e9364141661b 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> > >  	 * currently supports only 1 port per controller. So
> > >  	 * this is sufficient.
> > >  	 */
> > > +#ifdef CONFIG_USB
> > >  	udev = usb_hub_find_child(hcd->self.root_hub, 1);
> > > -
> > > +#else
> > > +	udev = NULL;
> > > +#endif
> > 
> > Perhaps the check should be moved to the caller instead? This function still
> > references "usb_hcd" struct and I don't think that's intended for gadget only
> > mode.
> 
> That wouldn't help with the build failure, which is what this patch is
> addressing.
> 

I should've put it clearly. You should guard the entire function and not just
usb_hub_find_child(). This way it becomes clear that this whole function depends
on the USB host functionality. Like,

#ifdef CONFIG_USB
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
{
...
}
#elif
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
{
	return USB_SPEED_UNKNOWN
}
#endif

Thanks,
Mani

> > >  	if (!udev)
> > >  		return USB_SPEED_UNKNOWN;
> > >  
> > > -- 
> > > 2.35.1
> 
> Johan
Johan Hovold Aug. 6, 2022, 4:51 p.m. UTC | #6
On Sat, Aug 06, 2022 at 10:12:50PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Aug 06, 2022 at 06:04:21PM +0200, Johan Hovold wrote:
> > On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > > > A recent change added a dependency to the USB host stack and broke
> > > > gadget-only builds of the driver.
> > > > 
> > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> > > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > > --`-
> > > > 
> > > > Changes in v2
> > > >  - new patch
> > > > 
> > > >  drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > > index be2e3dd36440..e9364141661b 100644
> > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> > > >  	 * currently supports only 1 port per controller. So
> > > >  	 * this is sufficient.
> > > >  	 */
> > > > +#ifdef CONFIG_USB
> > > >  	udev = usb_hub_find_child(hcd->self.root_hub, 1);
> > > > -
> > > > +#else
> > > > +	udev = NULL;
> > > > +#endif
> > > 
> > > Perhaps the check should be moved to the caller instead? This function still
> > > references "usb_hcd" struct and I don't think that's intended for gadget only
> > > mode.
> > 
> > That wouldn't help with the build failure, which is what this patch is
> > addressing.
> > 
> 
> I should've put it clearly. You should guard the entire function and not just
> usb_hub_find_child(). This way it becomes clear that this whole function depends
> on the USB host functionality. Like,
> 
> #ifdef CONFIG_USB
> static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> {
> ...
> }
> #elif
> static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> {
> 	return USB_SPEED_UNKNOWN
> }
> #endif

Yeah, that's what Krishna suggested but I wanted to keep the ifdeffery
minimal when fixing the build failure (we generally try to avoid adding
stub functions to implementation files).

Non-host mode was clearly never considered when adding the function in
question as the code blows up otherwise regardless of whether CONFIG_USB
is enabled or not (and a later patch in the series addresses that).

But I'll revisit this too in a couple of weeks.

Thanks for reviewing.

Johan
Greg Kroah-Hartman Aug. 18, 2022, 5:44 p.m. UTC | #7
On Mon, Aug 08, 2022 at 03:34:10PM +0200, Johan Hovold wrote:
> On Mon, Aug 08, 2022 at 03:05:36PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > > A recent change added a dependency to the USB host stack and broke
> > > gadget-only builds of the driver.
> > > 
> > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > > 
> > > Changes in v2
> > >  - new patch
> > > 
> > >  drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index be2e3dd36440..e9364141661b 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> > >  	 * currently supports only 1 port per controller. So
> > >  	 * this is sufficient.
> > >  	 */
> > > +#ifdef CONFIG_USB
> > >  	udev = usb_hub_find_child(hcd->self.root_hub, 1);
> > 
> > If a gadget driver needs this for some reason, then the #ifdef should be
> > put in a .h file, not in a .c file.
> 
> Yeah, if we're keeping this long-term then yes, and possibly also
> otherwise.
> 
> > But step back a minute and ask why a host-config-only function is being
> > called when a device is in gadget-only mode?  This feels like a
> > design/logic issue in this file, NOT something to paper over with a
> > #ifdef in a .c file
> 
> We're not as I'm fixing that bug in later in the series. I should
> probably have put this one after that fix, but figured fixing the build
> was more important than a harder-to-hit NULL-deref due to non-host mode
> not being considered when the offending series was merged.
> 
> > This implies that if this device is NOT in a host configuration, then
> > the suspend path of it is not configured properly at all, as why would
> > it be checking or caring about this at all if this is in gadget-only
> > mode?
> 
> Right, so see path 6/9 which addresses this by only calling this hack
> when in host mode:
> 
> 	https://lore.kernel.org/all/20220804151001.23612-7-johan+linaro@kernel.org/
> 
> > Something else is wrong here, let's fix the root problem please.  Maybe
> > this driver should just never be built in gadget-only mode, as it is
> > never intended to support that option?
> 
> The problem is commit 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup
> interrupts during suspend"), which I considered simply reverting but as
> that breaks suspend completely on some boards I decided to try and fix
> it up while we work on a proper long-term solution (i.e. for how the
> dwc/xhci layers should be communicating to implement this).
> 
> Remember that it took two years and 21 revisions to get to the state
> we're at now after you merged the wakeup series in June.

Very good point.  This is a mess, thanks for cleaning it up.  I've
applied this series now and will get it into 6.0-final, hopefully all
should be good now.

thanks for doing this work.

greg k-h