Sharing a USB controller with another entity via xhci-sideband driver
creates power management complexities. To prevent the USB controller
from being inadvertently deactivated while in use by the other entity, a
usage-count based mechanism is implemented. This allows the system to
manage power effectively, ensuring the controller remains available
whenever needed.

Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
 drivers/usb/core/driver.c         | 10 ++++++++++
 drivers/usb/core/hcd.c            |  1 +
 drivers/usb/core/usb.c            |  1 +
 drivers/usb/dwc3/core.c           | 13 +++++++++++++
 drivers/usb/dwc3/core.h           |  8 ++++++++
 drivers/usb/host/xhci-plat.c      | 10 ++++++++++
 drivers/usb/host/xhci-plat.h      |  7 +++++++
 sound/usb/qcom/qc_audio_offload.c |  3 +++
 8 files changed, 53 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index e713cf9b3dd2..eb85cbb1a2ff 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
 	struct usb_device	*udev = to_usb_device(dev);
 	int r;
 
+	if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) {
+		dev_dbg(dev, "device accessed via sideband\n");
+		return 0;
+	}
+
 	unbind_no_pm_drivers_interfaces(udev);
 
 	/* From now on we are sure all drivers support suspend/resume
@@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg)
 	struct usb_device	*udev = to_usb_device(dev);
 	int			status;
 
+	if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) {
+		dev_dbg(dev, "device accessed via sideband\n");
+		return 0;
+	}
+
 	/* For all calls, take the device back to full power and
 	 * tell the PM core in case it was autosuspended previously.
 	 * Unbind the interfaces that will need rebinding later,
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 1ff7d901fede..9876b3940281 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
 	timer_setup(&hcd->rh_timer, rh_timer_func, 0);
 #ifdef CONFIG_PM
 	INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
+	refcount_set(&hcd->sb_usage_count, 0);
 #endif
 
 	INIT_WORK(&hcd->died_work, hcd_died_work);
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 0b4685aad2d5..d315d066a56b 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
 	dev->state = USB_STATE_ATTACHED;
 	dev->lpm_disable_count = 1;
 	atomic_set(&dev->urbnum, 0);
+	refcount_set(&dev->sb_usage_count, 0);
 
 	INIT_LIST_HEAD(&dev->ep0.urb_list);
 	dev->ep0.desc.bLength = USB_DT_ENDPOINT_SIZE;
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2fdafbcbe44c..18c743ce5ac5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2550,8 +2550,15 @@ static int dwc3_runtime_idle(struct device *dev)
 static int dwc3_suspend(struct device *dev)
 {
 	struct dwc3	*dwc = dev_get_drvdata(dev);
+	struct platform_device *xhci = dwc->xhci;
 	int		ret;
 
+	if (xhci && xhci_sideband_check(xhci->dev.driver_data)) {
+		dev_dbg(dev, "device accessed via sideband\n");
+		return 0;
+	}
+
+
 	ret = dwc3_suspend_common(dwc, PMSG_SUSPEND);
 	if (ret)
 		return ret;
@@ -2564,8 +2571,14 @@ static int dwc3_suspend(struct device *dev)
 static int dwc3_resume(struct device *dev)
 {
 	struct dwc3	*dwc = dev_get_drvdata(dev);
+	struct platform_device *xhci = dwc->xhci;
 	int		ret;
 
+	if (xhci && xhci_sideband_check(xhci->dev.driver_data)) {
+		dev_dbg(dev, "device accessed via sideband\n");
+		return 0;
+	}
+
 	pinctrl_pm_select_default_state(dev);
 
 	pm_runtime_disable(dev);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 80047d0df179..e06d597ee3b0 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -26,6 +26,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/hcd.h>
 #include <linux/usb/role.h>
 #include <linux/ulpi/interface.h>
 
@@ -1704,4 +1705,11 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
 { }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND)
+extern bool xhci_sideband_check(struct usb_hcd *hcd);
+#else
+static inline bool xhci_sideband_check(struct usb_hcd *hcd)
+{ return false; }
+#endif
+
 #endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 6e49ef1908eb..5fdbdf0c7f1a 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -456,6 +456,11 @@ static int xhci_plat_suspend_common(struct device *dev, struct pm_message pmsg)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	int ret;
 
+	if (pmsg.event == PM_EVENT_SUSPEND && xhci_sideband_check(hcd)) {
+		dev_dbg(dev, "device accessed via sideband\n");
+		return 0;
+	}
+
 	if (pm_runtime_suspended(dev))
 		pm_runtime_resume(dev);
 
@@ -499,6 +504,11 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	int ret;
 
+	if (pmsg.event == PM_EVENT_RESUME && xhci_sideband_check(hcd)) {
+		dev_dbg(dev, "device accessed via sideband\n");
+		return 0;
+	}
+
 	if (!device_may_wakeup(dev) && (xhci->quirks & XHCI_SUSPEND_RESUME_CLKS)) {
 		ret = clk_prepare_enable(xhci->clk);
 		if (ret)
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 6475130eac4b..432a040c81e5 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -30,4 +30,11 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev,
 void xhci_plat_remove(struct platform_device *dev);
 extern const struct dev_pm_ops xhci_plat_pm_ops;
 
+#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND)
+extern bool xhci_sideband_check(struct usb_hcd *hcd);
+#else
+static inline bool xhci_sideband_check(struct usb_hcd *hcd)
+{ return false; }
+#endif
+
 #endif	/* _XHCI_PLAT_H */
diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index 2dc651cd3d05..c82b5dbef2d7 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -1516,8 +1516,11 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
 			mutex_lock(&chip->mutex);
 			subs->opened = 0;
 			mutex_unlock(&chip->mutex);
+		} else {
+			xhci_sideband_get(uadev[pcm_card_num].sb);
 		}
 	} else {
+		xhci_sideband_put(uadev[pcm_card_num].sb);
 		info = &uadev[pcm_card_num].info[info_idx];
 		if (info->data_ep_pipe) {
 			ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
-- 
2.47.0.rc1.288.g06298d1525-goog

On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote:
> Sharing a USB controller with another entity via xhci-sideband driver
> creates power management complexities. To prevent the USB controller
> from being inadvertently deactivated while in use by the other entity, a
> usage-count based mechanism is implemented. This allows the system to
> manage power effectively, ensuring the controller remains available
> whenever needed.
> 
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> ---
>  drivers/usb/core/driver.c         | 10 ++++++++++
>  drivers/usb/core/hcd.c            |  1 +
>  drivers/usb/core/usb.c            |  1 +
>  drivers/usb/dwc3/core.c           | 13 +++++++++++++
>  drivers/usb/dwc3/core.h           |  8 ++++++++
>  drivers/usb/host/xhci-plat.c      | 10 ++++++++++
>  drivers/usb/host/xhci-plat.h      |  7 +++++++
>  sound/usb/qcom/qc_audio_offload.c |  3 +++
>  8 files changed, 53 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index e713cf9b3dd2..eb85cbb1a2ff 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
>  	struct usb_device	*udev = to_usb_device(dev);
>  	int r;
>  
> +	if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) {
> +		dev_dbg(dev, "device accessed via sideband\n");
> +		return 0;
> +	}

I'm not so sure about this.  By returning early, you prevent the drivers 
bound to this device from suspending.  But they can't operate properly 
when the system is in a low-power mode.  Won't that cause problems?

Maybe this really belongs in usb_suspend_device(), and its counterpart 
belongs in usb_resume_device().

> +
>  	unbind_no_pm_drivers_interfaces(udev);
>  
>  	/* From now on we are sure all drivers support suspend/resume
> @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg)
>  	struct usb_device	*udev = to_usb_device(dev);
>  	int			status;
>  
> +	if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) {
> +		dev_dbg(dev, "device accessed via sideband\n");
> +		return 0;
> +	}
> +
>  	/* For all calls, take the device back to full power and
>  	 * tell the PM core in case it was autosuspended previously.
>  	 * Unbind the interfaces that will need rebinding later,
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 1ff7d901fede..9876b3940281 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
>  	timer_setup(&hcd->rh_timer, rh_timer_func, 0);
>  #ifdef CONFIG_PM
>  	INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
> +	refcount_set(&hcd->sb_usage_count, 0);

Did I miss something?  I didn't notice this field in any of the earlier 
patches.  Was it already created by the prerequisite series?  If so, why 
didn't that series do this initialization?

>  #endif
>  
>  	INIT_WORK(&hcd->died_work, hcd_died_work);
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 0b4685aad2d5..d315d066a56b 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	dev->state = USB_STATE_ATTACHED;
>  	dev->lpm_disable_count = 1;
>  	atomic_set(&dev->urbnum, 0);
> +	refcount_set(&dev->sb_usage_count, 0);

And doesn't this belong in the 3/5 patch, the one that creates the 
sb_usage_count field?

Alan Stern

On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote:
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index e713cf9b3dd2..eb85cbb1a2ff 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> >       struct usb_device       *udev = to_usb_device(dev);
> >       int r;
> >
> > +     if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) {
> > +             dev_dbg(dev, "device accessed via sideband\n");
> > +             return 0;
> > +     }
>
> I'm not so sure about this.  By returning early, you prevent the drivers
> bound to this device from suspending.  But they can't operate properly
> when the system is in a low-power mode.  Won't that cause problems?
>
> Maybe this really belongs in usb_suspend_device(), and its counterpart
> belongs in usb_resume_device().
>

To my understanding, after the system is suspended, the USB driver
will do nothing as the main processor has been suspended. May I check
what forms of low power mode and operation we are discussing here?

usb_suspend_device() did close the required port/bus to maintain usb
transfer. However, there are additional usb_hcd_flush_endpoint() calls
aside from usb_suspend_device(). Maybe we should consider not doing
those also since some of the endpoints are now handled by the
sideband.

> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 1ff7d901fede..9876b3940281 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> >       timer_setup(&hcd->rh_timer, rh_timer_func, 0);
> >  #ifdef CONFIG_PM
> >       INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
> > +     refcount_set(&hcd->sb_usage_count, 0);
>
> Did I miss something?  I didn't notice this field in any of the earlier
> patches.  Was it already created by the prerequisite series?  If so, why
> didn't that series do this initialization?
>
> >  #endif
> >
> >       INIT_WORK(&hcd->died_work, hcd_died_work);
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 0b4685aad2d5..d315d066a56b 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> >       dev->state = USB_STATE_ATTACHED;
> >       dev->lpm_disable_count = 1;
> >       atomic_set(&dev->urbnum, 0);
> > +     refcount_set(&dev->sb_usage_count, 0);
>
> And doesn't this belong in the 3/5 patch, the one that creates the
> sb_usage_count field?
>
> Alan Stern

Thanks for the suggestion, I'll move this, as well as the
initialization of hcd->sb_usage_count, to corresponding earlier
patches.

Regards,
Guan-Yu

On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote:
> On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > I'm not so sure about this.  By returning early, you prevent the drivers
> > bound to this device from suspending.  But they can't operate properly
> > when the system is in a low-power mode.  Won't that cause problems?
> >
> > Maybe this really belongs in usb_suspend_device(), and its counterpart
> > belongs in usb_resume_device().
> >
> 
> To my understanding, after the system is suspended, the USB driver
> will do nothing as the main processor has been suspended. May I check
> what forms of low power mode and operation we are discussing here?

S3 suspend.  You are right that the driver will do nothing while the
CPU is suspended.  But what about the times before and after that,
while the suspend and resume procedures are underway?  The driver
needs to be told to cancel any ongoing transfers while the system
suspends and then restart them while the system resumes.

> usb_suspend_device() did close the required port/bus to maintain usb
> transfer.

I don't know what you mean by that.

>  However, there are additional usb_hcd_flush_endpoint() calls
> aside from usb_suspend_device(). Maybe we should consider not doing
> those also since some of the endpoints are now handled by the
> sideband.

Those calls should not be skipped.  If the sideband controller wants
to handle the endpoints on its own, it can go right ahead.  The main
controller and the USB core need to know that they shouldn't use the
endpoint while the system is suspending.

Alan Stern

On Tue, Oct 15, 2024 at 10:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote:
> > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > I'm not so sure about this.  By returning early, you prevent the drivers
> > > bound to this device from suspending.  But they can't operate properly
> > > when the system is in a low-power mode.  Won't that cause problems?
> > >
> > > Maybe this really belongs in usb_suspend_device(), and its counterpart
> > > belongs in usb_resume_device().
> > >
> >
> > To my understanding, after the system is suspended, the USB driver
> > will do nothing as the main processor has been suspended. May I check
> > what forms of low power mode and operation we are discussing here?
>
> S3 suspend.  You are right that the driver will do nothing while the
> CPU is suspended.  But what about the times before and after that,
> while the suspend and resume procedures are underway?  The driver
> needs to be told to cancel any ongoing transfers while the system
> suspends and then restart them while the system resumes.
>

Regarding the cancellation of ongoing transfers during suspend, I
believe usb_hcd_flush_endpoint() handles this as discussed below.
Besides calling usb_hcd_flush_endpoint(), are there any other
necessary changes before suspending the driver in our scenario? Maybe
we could discuss setting usb_device_state to USB_STATE_SUSPENDED.
However, my understanding is that this variable reflects the actual
device state. Since the device remains active via the sideband in our
case,  changing usb_device_state seems unnecessary.

> > usb_suspend_device() did close the required port/bus to maintain usb
> > transfer.
>
> I don't know what you mean by that.
>

Nothing special here, I'm just echoing what you've mentioned and
trying to bring up the discussion about usb_hcd_flush_endpoint().

> >  However, there are additional usb_hcd_flush_endpoint() calls
> > aside from usb_suspend_device(). Maybe we should consider not doing
> > those also since some of the endpoints are now handled by the
> > sideband.
>
> Those calls should not be skipped.  If the sideband controller wants
> to handle the endpoints on its own, it can go right ahead.  The main
> controller and the USB core need to know that they shouldn't use the
> endpoint while the system is suspending.
>
> Alan Stern

Got it, let me update the patch and put the related changes into
usb_suspend_device()/usb_resume_device().

Regards,
Guan-Yu

On Wed, Oct 16, 2024 at 03:40:00PM +0800, Guan-Yu Lin wrote:
> On Tue, Oct 15, 2024 at 10:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote:
> > > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > I'm not so sure about this.  By returning early, you prevent the drivers
> > > > bound to this device from suspending.  But they can't operate properly
> > > > when the system is in a low-power mode.  Won't that cause problems?
> > > >
> > > > Maybe this really belongs in usb_suspend_device(), and its counterpart
> > > > belongs in usb_resume_device().
> > > >
> > >
> > > To my understanding, after the system is suspended, the USB driver
> > > will do nothing as the main processor has been suspended. May I check
> > > what forms of low power mode and operation we are discussing here?
> >
> > S3 suspend.  You are right that the driver will do nothing while the
> > CPU is suspended.  But what about the times before and after that,
> > while the suspend and resume procedures are underway?  The driver
> > needs to be told to cancel any ongoing transfers while the system
> > suspends and then restart them while the system resumes.
> >
> 
> Regarding the cancellation of ongoing transfers during suspend, I
> believe usb_hcd_flush_endpoint() handles this as discussed below.

There's more to it than that.  If you cancel ongoing transfers by 
calling usb_hcd_flush_endpoint() without informing the driver first, the 
driver will get very confused and think the device has failed.

> Besides calling usb_hcd_flush_endpoint(), are there any other
> necessary changes before suspending the driver in our scenario? Maybe
> we could discuss setting usb_device_state to USB_STATE_SUSPENDED.
> However, my understanding is that this variable reflects the actual
> device state. Since the device remains active via the sideband in our
> case,  changing usb_device_state seems unnecessary.

That's right.

I don't think anything else is needed.  Just call 
usb_suspend_interface() like the normal pathway in usb_suspend_both() 
does, but skip calling usb_suspend_device().

Alan Stern

On Wed, Oct 16, 2024 at 10:45 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Oct 16, 2024 at 03:40:00PM +0800, Guan-Yu Lin wrote:
> > On Tue, Oct 15, 2024 at 10:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote:
> > > > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > I'm not so sure about this.  By returning early, you prevent the drivers
> > > > > bound to this device from suspending.  But they can't operate properly
> > > > > when the system is in a low-power mode.  Won't that cause problems?
> > > > >
> > > > > Maybe this really belongs in usb_suspend_device(), and its counterpart
> > > > > belongs in usb_resume_device().
> > > > >
> > > >
> > > > To my understanding, after the system is suspended, the USB driver
> > > > will do nothing as the main processor has been suspended. May I check
> > > > what forms of low power mode and operation we are discussing here?
> > >
> > > S3 suspend.  You are right that the driver will do nothing while the
> > > CPU is suspended.  But what about the times before and after that,
> > > while the suspend and resume procedures are underway?  The driver
> > > needs to be told to cancel any ongoing transfers while the system
> > > suspends and then restart them while the system resumes.
> > >
> >
> > Regarding the cancellation of ongoing transfers during suspend, I
> > believe usb_hcd_flush_endpoint() handles this as discussed below.
>
> There's more to it than that.  If you cancel ongoing transfers by
> calling usb_hcd_flush_endpoint() without informing the driver first, the
> driver will get very confused and think the device has failed.
>
> > Besides calling usb_hcd_flush_endpoint(), are there any other
> > necessary changes before suspending the driver in our scenario? Maybe
> > we could discuss setting usb_device_state to USB_STATE_SUSPENDED.
> > However, my understanding is that this variable reflects the actual
> > device state. Since the device remains active via the sideband in our
> > case,  changing usb_device_state seems unnecessary.
>
> That's right.
>
> I don't think anything else is needed.  Just call
> usb_suspend_interface() like the normal pathway in usb_suspend_both()
> does, but skip calling usb_suspend_device().
>
> Alan Stern

Thanks for the suggestions, let me address them in the next version.
After some local development, our experiments suggest it may be
necessary to skip usb_suspend_interface() & usb_hcd_flush_endpoint()
for connection changes behind a hub and HID events in our scenario.

Typically, when the system sleeps, the hub uses remote wakeup to
reactivate upstream devices and resume the interface to handle
connection changes. However, our current conclusion is to maintain the
device in an active state while suspending the interface. This
deviates from the norm, as remote wakeup is designed to function when
devices and links are suspended. We're concerned that this discrepancy
might interfere with the remote wakeup mechanism.
To address this, we're currently bypassing usb_suspend_interface() and
usb_hcd_flush_endpoint(). This effectively simulates an "active
system" state, allowing the USB controller to notify the kernel about
connection changes via interrupts. This workaround applies to HID
events as well.

Which approach do you recommend? Should we invest in integrating with
the remote wakeup framework, or is it acceptable to keep necessary
components active, mirroring an "active system" state?

Regards,
Guan-Yu

On Fri, Oct 18, 2024 at 07:59:00PM +0800, Guan-Yu Lin wrote:
> Thanks for the suggestions, let me address them in the next version.
> After some local development, our experiments suggest it may be
> necessary to skip usb_suspend_interface() & usb_hcd_flush_endpoint()
> for connection changes behind a hub and HID events in our scenario.
> 
> Typically, when the system sleeps, the hub uses remote wakeup to
> reactivate upstream devices and resume the interface to handle
> connection changes. However, our current conclusion is to maintain the
> device in an active state while suspending the interface. This
> deviates from the norm, as remote wakeup is designed to function when
> devices and links are suspended. We're concerned that this discrepancy
> might interfere with the remote wakeup mechanism.
> To address this, we're currently bypassing usb_suspend_interface() and
> usb_hcd_flush_endpoint(). This effectively simulates an "active
> system" state, allowing the USB controller to notify the kernel about
> connection changes via interrupts. This workaround applies to HID
> events as well.
> 
> Which approach do you recommend? Should we invest in integrating with
> the remote wakeup framework, or is it acceptable to keep necessary
> components active, mirroring an "active system" state?

It's hard to answer those questions because I don't have a clear idea of 
how the sideband system is meant to work.  For instance, what does the 
sideband system do when a port-connect change is detected in a hub that 
sits between the computer and the audio device?  If the sideband system 
decides to change the audio device's settings, how does the regular 
audio driver learn about the change?  And so on.

It's worth pointing out that allowing two different drivers to control 
the same device is generally not a good idea.  More likely than not they 
will end up interfering with each other at some stage.

Alan Stern

On Mon, Oct 14, 2024 at 11:56:40AM -0400, Alan Stern wrote:
> On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote:

> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 1ff7d901fede..9876b3940281 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> >  	timer_setup(&hcd->rh_timer, rh_timer_func, 0);
> >  #ifdef CONFIG_PM
> >  	INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
> > +	refcount_set(&hcd->sb_usage_count, 0);
> 
> Did I miss something?  I didn't notice this field in any of the earlier 
> patches.  Was it already created by the prerequisite series?  If so, why 
> didn't that series do this initialization?

Oops, yes, I did miss it -- it's in the 4/5 patch.  Sorry.

Alan Stern

On 14.10.2024 11.50, Guan-Yu Lin wrote:
> Sharing a USB controller with another entity via xhci-sideband driver
> creates power management complexities. To prevent the USB controller
> from being inadvertently deactivated while in use by the other entity, a
> usage-count based mechanism is implemented. This allows the system to
> manage power effectively, ensuring the controller remains available
> whenever needed.

I don't think all this is needed to prevent USB controller from being
deactivated while sideband is in use. The modified audio class driver
that uses sideband is still bound to a usb interface device, and all
normal pm reference counting should work.

To me it looks like this code is tricking pm framework into believing
the usb device and host controller have successfully suspended during
system suspend when they in reality are fully up and running.

I'm not sure I fully understand the case this series is solving.

Thanks
Mathias

On Mon, Oct 14, 2024 at 9:06 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 14.10.2024 11.50, Guan-Yu Lin wrote:
> > Sharing a USB controller with another entity via xhci-sideband driver
> > creates power management complexities. To prevent the USB controller
> > from being inadvertently deactivated while in use by the other entity, a
> > usage-count based mechanism is implemented. This allows the system to
> > manage power effectively, ensuring the controller remains available
> > whenever needed.
>
> I don't think all this is needed to prevent USB controller from being
> deactivated while sideband is in use. The modified audio class driver
> that uses sideband is still bound to a usb interface device, and all
> normal pm reference counting should work.
>
> To me it looks like this code is tricking pm framework into believing
> the usb device and host controller have successfully suspended during
> system suspend when they in reality are fully up and running.
>
> I'm not sure I fully understand the case this series is solving.
>
> Thanks
> Mathias
>

Your understanding is correct; this series aims to keep necessary
devices running while the system is suspended. The goal is to keep USB
transfer available via sideband when the main system is asleep. Since
we're offloading some USB transfers to another audio DSP, the Linux
driver is not involved in queueing transfer TRBs, handling event TRBs,
etc., in some specific "offloaded" USB endpoints. The sideband might
be bound to a USB interface device to prevent the device from going
into runtime suspend, but it seems this doesn't prevent devices from
suspending when the system goes into system-wide suspend. So the main
purpose of this series is to prevent some related devices being
suspended during system suspend.

Regards,
Guan-Yu

On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote:
> Sharing a USB controller with another entity via xhci-sideband driver
> creates power management complexities. To prevent the USB controller
> from being inadvertently deactivated while in use by the other entity, a
> usage-count based mechanism is implemented. This allows the system to
> manage power effectively, ensuring the controller remains available
> whenever needed.
> 
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> ---
>  drivers/usb/core/driver.c         | 10 ++++++++++
>  drivers/usb/core/hcd.c            |  1 +
>  drivers/usb/core/usb.c            |  1 +
>  drivers/usb/dwc3/core.c           | 13 +++++++++++++
>  drivers/usb/dwc3/core.h           |  8 ++++++++
>  drivers/usb/host/xhci-plat.c      | 10 ++++++++++
>  drivers/usb/host/xhci-plat.h      |  7 +++++++
>  sound/usb/qcom/qc_audio_offload.c |  3 +++
>  8 files changed, 53 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index e713cf9b3dd2..eb85cbb1a2ff 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
>  	struct usb_device	*udev = to_usb_device(dev);
>  	int r;
>  
> +	if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) {
> +		dev_dbg(dev, "device accessed via sideband\n");
> +		return 0;
> +	}

What prevents the check from changing state right after you call this?

> +
>  	unbind_no_pm_drivers_interfaces(udev);
>  
>  	/* From now on we are sure all drivers support suspend/resume
> @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg)
>  	struct usb_device	*udev = to_usb_device(dev);
>  	int			status;
>  
> +	if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) {
> +		dev_dbg(dev, "device accessed via sideband\n");
> +		return 0;
> +	}

Same here, what's keeping the state from changing?


> +
>  	/* For all calls, take the device back to full power and
>  	 * tell the PM core in case it was autosuspended previously.
>  	 * Unbind the interfaces that will need rebinding later,
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 1ff7d901fede..9876b3940281 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
>  	timer_setup(&hcd->rh_timer, rh_timer_func, 0);
>  #ifdef CONFIG_PM
>  	INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
> +	refcount_set(&hcd->sb_usage_count, 0);
>  #endif
>  
>  	INIT_WORK(&hcd->died_work, hcd_died_work);
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 0b4685aad2d5..d315d066a56b 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	dev->state = USB_STATE_ATTACHED;
>  	dev->lpm_disable_count = 1;
>  	atomic_set(&dev->urbnum, 0);
> +	refcount_set(&dev->sb_usage_count, 0);
>  
>  	INIT_LIST_HEAD(&dev->ep0.urb_list);
>  	dev->ep0.desc.bLength = USB_DT_ENDPOINT_SIZE;
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2fdafbcbe44c..18c743ce5ac5 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2550,8 +2550,15 @@ static int dwc3_runtime_idle(struct device *dev)
>  static int dwc3_suspend(struct device *dev)
>  {
>  	struct dwc3	*dwc = dev_get_drvdata(dev);
> +	struct platform_device *xhci = dwc->xhci;
>  	int		ret;
>  
> +	if (xhci && xhci_sideband_check(xhci->dev.driver_data)) {

What could go wrong with poking into a random device structure's private
data that you don't know the type of?  :(

> +		dev_dbg(dev, "device accessed via sideband\n");
> +		return 0;

I predict, that if this all does get implemented, we're going to have a
lot of confusion of "why will my devices not go into suspend?"
questions, right?

How does userspace know if a device is controlled by a sideband path or
not?  Is there some sysfs link somewhere, and does any tool show it
anyway?

thanks,

greg k-h

On Mon, Oct 14, 2024 at 5:21 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote:
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index e713cf9b3dd2..eb85cbb1a2ff 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> >       struct usb_device       *udev = to_usb_device(dev);
> >       int r;
> >
> > +     if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) {
> > +             dev_dbg(dev, "device accessed via sideband\n");
> > +             return 0;
> > +     }
>
> What prevents the check from changing state right after you call this?
>

Maybe we should hold a lock before doing the check, and release the
lock after the entire system suspend period to ensure no sideband will
access the USB device after it has been suspended. However, I'm not
very confident about holding a lock over the entire suspend period.
Should we do so or use a boolean flag instead? I'm not sure if holding
a lock will prevent the system from entering low-power state or not.

> > +
> >       unbind_no_pm_drivers_interfaces(udev);
> >
> >       /* From now on we are sure all drivers support suspend/resume
> > @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg)
> >       struct usb_device       *udev = to_usb_device(dev);
> >       int                     status;
> >
> > +     if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) {
> > +             dev_dbg(dev, "device accessed via sideband\n");
> > +             return 0;
> > +     }
>
> Same here, what's keeping the state from changing?
>
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 2fdafbcbe44c..18c743ce5ac5 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -2550,8 +2550,15 @@ static int dwc3_runtime_idle(struct device *dev)
> >  static int dwc3_suspend(struct device *dev)
> >  {
> >       struct dwc3     *dwc = dev_get_drvdata(dev);
> > +     struct platform_device *xhci = dwc->xhci;
> >       int             ret;
> >
> > +     if (xhci && xhci_sideband_check(xhci->dev.driver_data)) {
>
> What could go wrong with poking into a random device structure's private
> data that you don't know the type of?  :(
>

We did observe that the driver_data was set to struct usb_hcd in the
"platform:xhci-hcd" module, which was the default linux xhci host
controller platform glue driver. Should we rewrite as "struct usb_hcd
*hcd = dev_get_drvdata(dev);" and then operate with hcd? Or even with
this kind of statement the idea is still unacceptable?



> > +             dev_dbg(dev, "device accessed via sideband\n");
> > +             return 0;
>
> I predict, that if this all does get implemented, we're going to have a
> lot of confusion of "why will my devices not go into suspend?"
> questions, right?
>

This feature should not influence anyone not using xhci sideband
framework. For those who use xhci sideband framework, the current
design seems to not support USB transfer during system suspend. I
think the goal for this patch is to enable system suspend while
holding necessary devices active to enable USB transfer via sideband,
and suspend all devices if we don't have USB transfers on it. The USB
devices are active only when there are USB transfers happening on it.
Based on this, I think users will have less concern on why the USB
devices are still active.

> How does userspace know if a device is controlled by a sideband path or
> not?  Is there some sysfs link somewhere, and does any tool show it
> anyway?
>
> thanks,
>
> greg k-h

I don't recall there's a mechanism to show sideband activity to
userspace. Is this need solely related to enabling USB transfer via
sideband during system suspend? Or is it related to generic sideband
functionality? If the latter is the case, maybe we should propose a
patch in series "Introduce QC USB SND audio offloading support" to
support this mechanism.

Regards,
Guan-Yu