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/dwc3/core.c      | 20 ++++++++++++++++++++
 drivers/usb/dwc3/core.h      |  1 +
 drivers/usb/host/xhci-plat.c | 10 ++++++++++
 include/linux/usb/hcd.h      |  7 +++++++
 5 files changed, 48 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index e53cb4c267b3..e5bb26e6c71a 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/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2fdafbcbe44c..d85c68d5eba4 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2550,8 +2550,18 @@ 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;
+	struct usb_hcd  *hcd;
 	int		ret;
 
+	if (xhci) {
+		hcd = dev_get_drvdata(&xhci->dev);
+		if (xhci_sideband_check(hcd)) {
+			dev_dbg(dev, "device accessed via sideband\n");
+			return 0;
+		}
+	}
+
 	ret = dwc3_suspend_common(dwc, PMSG_SUSPEND);
 	if (ret)
 		return ret;
@@ -2564,8 +2574,18 @@ 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;
+	struct usb_hcd  *hcd;
 	int		ret;
 
+	if (xhci) {
+		hcd = dev_get_drvdata(&xhci->dev);
+		if (xhci_sideband_check(hcd)) {
+			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..a585e9d80e59 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>
 
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/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 9867c178d188..b22d25ccdf7c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -772,6 +772,13 @@ extern struct rw_semaphore ehci_cf_port_reset_rwsem;
 #define USB_EHCI_LOADED		2
 extern unsigned long usb_hcds_loaded;
 
+#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 /* __KERNEL__ */
 
 #endif /* __USB_CORE_HCD_H */
-- 
2.47.0.199.ga7371fff76-goog

On Wed, Nov 6, 2024 at 4:35 PM Guan-Yu Lin <guanyulin@google.com> wrote:
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index e53cb4c267b3..e5bb26e6c71a 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,

In v5, Greg points out the race window between checking sideband
activity and handling power management of usb devices. We should
consider a lock mechanism to address the race window. Given that the
design hasn't locked down and the race window might change from time
to time. I'll address this after the discussion of suspending USB
devices/interfaces has converged.

In addition, Alan suggests to only keep USB devices active but suspend
the USB interfaces. However, hub events and key events require active
USB interfaces to function. In the sideband model, the sideband driver
only handles USB transfers on specific endpoints, leaving other
functionalities like connection changes and key events to the Linux
USB kernel drivers. Therefore, a potential design modification is to
shift the sideband model's focus from voting for USB devices to voting
for USB interfaces. This way, the driver could selectively hold
necessary interfaces active during system suspend. This adjustment
accommodates use cases where specific interfaces must remain active to
support overall USB functionality when partial interfaces are
offloaded to a sideband.