[PATCH v5 0/5] Support system sleep with offloaded usb transfers

[PATCH v5 0/5] Support system sleep with offloaded usb transfers

[Guan-Yu Lin]

Wesley Cheng and Mathias Nyman's USB offload design enables a co-processor
to handle some USB transfers, potentially allowing the main system to
sleep and save power. However, Linux's power management system halts the
USB host controller when the main system isn't managing any USB transfers.
To address this, the proposal modifies the system to recognize offloaded
USB transfers and manage power accordingly.

This involves two key steps:
1. Transfer Status Tracking: Propose xhci_sideband_get and
xhci_sideband_put to track USB transfers on the co-processor, ensuring the
system is aware of any ongoing activity.
2. Power Management Adjustment:  Modifications to the USB driver stack
(dwc3 controller driver, xhci host controller driver, and USB device
drivers) allow the system to sleep without disrupting co-processor managed
USB transfers. This involves adding conditional checks to bypass some
power management operations.

patches depends on series "Introduce QC USB SND audio offloading support" 
https://lore.kernel.org/lkml/20240925010000.2231406-11-quic_wcheng@quicinc.com/T/

changelog
----------
Changes in v5:
- Walk through the USB children in usb_sideband_check() to determine the
  sideband activity under the specific USB device. 
- Replace atomic_t by refcount_t.
- Reduce logs by using dev_dbg & remove __func__.

Changes in v4:
- Isolate the feature into USB driver stack.
- Integrate with series "Introduce QC USB SND audio offloading support"

Changes in v3:
- Integrate the feature with the pm core framework.

Changes in v2:
- Cosmetics changes on coding style.

[v3] PM / core: conditionally skip system pm in device/driver model
[v2] usb: host: enable suspend-to-RAM control in userspace
[v1] [RFC] usb: host: Allow userspace to control usb suspend flows
---

Guan-Yu Lin (5):
  usb: dwc3: separate dev_pm_ops for each pm_event
  usb: xhci-plat: separate dev_pm_ops for each pm_event
  usb: add apis for sideband usage tracking
  xhci: sideband: add api to trace sideband usage
  usb: host: enable sideband transfer during system sleep

 drivers/usb/core/driver.c         | 63 ++++++++++++++++++++++
 drivers/usb/core/hcd.c            |  1 +
 drivers/usb/core/usb.c            |  1 +
 drivers/usb/dwc3/core.c           | 90 ++++++++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h           |  8 +++
 drivers/usb/host/xhci-plat.c      | 38 +++++++++++--
 drivers/usb/host/xhci-plat.h      |  7 +++
 drivers/usb/host/xhci-sideband.c  | 74 +++++++++++++++++++++++++
 include/linux/usb.h               | 13 +++++
 include/linux/usb/hcd.h           |  4 ++
 include/linux/usb/xhci-sideband.h |  5 ++
 sound/usb/qcom/qc_audio_offload.c |  3 ++
 12 files changed, 302 insertions(+), 5 deletions(-)

-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v5 1/5] usb: dwc3: separate dev_pm_ops for each pm_event

[Guan-Yu Lin]

Separate dev_pm_ops for different power events such as suspend, thaw,
and hibernation. This is crucial when dwc3 driver needs to adapt its
behavior based on different power state changes.

Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
 drivers/usb/dwc3/core.c | 77 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b25d80f318a9..2fdafbcbe44c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2582,6 +2582,76 @@ static int dwc3_resume(struct device *dev)
 	return 0;
 }
 
+static int dwc3_freeze(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	int		ret;
+
+	ret = dwc3_suspend_common(dwc, PMSG_FREEZE);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int dwc3_thaw(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	int		ret;
+
+	pinctrl_pm_select_default_state(dev);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+
+	ret = dwc3_resume_common(dwc, PMSG_THAW);
+	if (ret) {
+		pm_runtime_set_suspended(dev);
+		return ret;
+	}
+
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static int dwc3_poweroff(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	int		ret;
+
+	ret = dwc3_suspend_common(dwc, PMSG_HIBERNATE);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int dwc3_restore(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	int		ret;
+
+	pinctrl_pm_select_default_state(dev);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+
+	ret = dwc3_resume_common(dwc, PMSG_RESTORE);
+	if (ret) {
+		pm_runtime_set_suspended(dev);
+		return ret;
+	}
+
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
 static void dwc3_complete(struct device *dev)
 {
 	struct dwc3	*dwc = dev_get_drvdata(dev);
@@ -2599,7 +2669,12 @@ static void dwc3_complete(struct device *dev)
 #endif /* CONFIG_PM_SLEEP */
 
 static const struct dev_pm_ops dwc3_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
+	.suspend = pm_sleep_ptr(dwc3_suspend),
+	.resume = pm_sleep_ptr(dwc3_resume),
+	.freeze = pm_sleep_ptr(dwc3_freeze),
+	.thaw = pm_sleep_ptr(dwc3_thaw),
+	.poweroff = pm_sleep_ptr(dwc3_poweroff),
+	.restore = pm_sleep_ptr(dwc3_restore),
 	.complete = dwc3_complete,
 	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
 			dwc3_runtime_idle)
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v5 2/5] usb: xhci-plat: separate dev_pm_ops for each pm_event

[Guan-Yu Lin]

Separate dev_pm_ops for different power events such as suspend, thaw,
and hibernation. This is crucial when xhci-plat driver needs to adapt
its behavior based on different power state changes.

Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
 drivers/usb/host/xhci-plat.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 8dc23812b204..6e49ef1908eb 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -450,7 +450,7 @@ void xhci_plat_remove(struct platform_device *dev)
 }
 EXPORT_SYMBOL_GPL(xhci_plat_remove);
 
-static int xhci_plat_suspend(struct device *dev)
+static int xhci_plat_suspend_common(struct device *dev, struct pm_message pmsg)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
@@ -478,6 +478,21 @@ static int xhci_plat_suspend(struct device *dev)
 	return 0;
 }
 
+static int xhci_plat_suspend(struct device *dev)
+{
+	return xhci_plat_suspend_common(dev, PMSG_SUSPEND);
+}
+
+static int xhci_plat_freeze(struct device *dev)
+{
+	return xhci_plat_suspend_common(dev, PMSG_FREEZE);
+}
+
+static int xhci_plat_poweroff(struct device *dev)
+{
+	return xhci_plat_suspend_common(dev, PMSG_HIBERNATE);
+}
+
 static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
@@ -524,6 +539,11 @@ static int xhci_plat_resume(struct device *dev)
 	return xhci_plat_resume_common(dev, PMSG_RESUME);
 }
 
+static int xhci_plat_thaw(struct device *dev)
+{
+	return xhci_plat_resume_common(dev, PMSG_THAW);
+}
+
 static int xhci_plat_restore(struct device *dev)
 {
 	return xhci_plat_resume_common(dev, PMSG_RESTORE);
@@ -553,9 +573,9 @@ static int __maybe_unused xhci_plat_runtime_resume(struct device *dev)
 const struct dev_pm_ops xhci_plat_pm_ops = {
 	.suspend = pm_sleep_ptr(xhci_plat_suspend),
 	.resume = pm_sleep_ptr(xhci_plat_resume),
-	.freeze = pm_sleep_ptr(xhci_plat_suspend),
-	.thaw = pm_sleep_ptr(xhci_plat_resume),
-	.poweroff = pm_sleep_ptr(xhci_plat_suspend),
+	.freeze = pm_sleep_ptr(xhci_plat_freeze),
+	.thaw = pm_sleep_ptr(xhci_plat_thaw),
+	.poweroff = pm_sleep_ptr(xhci_plat_poweroff),
 	.restore = pm_sleep_ptr(xhci_plat_restore),
 
 	SET_RUNTIME_PM_OPS(xhci_plat_runtime_suspend,
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v5 3/5] usb: add apis for sideband usage tracking

[Guan-Yu Lin]

Introduce sb_usage_count and corresponding apis to track sideband usage
on each USB device. A sideband refers to the co-processor that accesses
the usb_device via shared control on the same USB host controller. To
optimize power usage, it's essential to monitor whether ther sideband is
actively using the USB device. This information is vital when
determining if a USB device can be safely suspended during system power
state transitions.

Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
 drivers/usb/core/driver.c | 53 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h       | 13 ++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 0c3f12daac79..e713cf9b3dd2 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1673,6 +1673,59 @@ void usb_disable_autosuspend(struct usb_device *udev)
 }
 EXPORT_SYMBOL_GPL(usb_disable_autosuspend);
 
+/**
+ * usb_sideband_get - increment the sb_usage_count of a USB device
+ * @udev: the USB device to increment its sb_usage_count
+ *
+ * Incrementing the sb_usage_count of a usb_device indicates that a sideband is
+ * currently using the device; that is, another entity is actively handling USB
+ * transfers. This information allows the USB driver to adjust its power
+ * management policy based on sideband activity.
+ */
+void usb_sideband_get(struct usb_device *udev)
+{
+	refcount_inc(&udev->sb_usage_count);
+}
+EXPORT_SYMBOL_GPL(usb_sideband_get);
+
+/**
+ * usb_sideband_put - drop the sb_usage_count of a USB device
+ * @udev: the USB device to drop its sb_usage_count
+ *
+ * The inverse operation of usb_sideband_get, which drops the sb_usage_count of
+ * a USB device. This information allows the USB driver to adjust its power
+ * management policy based on sideband activity.
+ */
+void usb_sideband_put(struct usb_device *udev)
+{
+	refcount_dec(&udev->sb_usage_count);
+}
+EXPORT_SYMBOL_GPL(usb_sideband_put);
+
+/**
+ * usb_sideband_check - check sideband activities on a USB device
+ * @udev: the USB device to check its sideband activity.
+ *
+ * Check if there are any sideband activity on the USB device right now. This
+ * information could be used for power management or other forms or resource
+ * management.
+ *
+ * Returns true on any active sideband existence, false otherwise
+ */
+bool usb_sideband_check(struct usb_device *udev)
+{
+	struct usb_device *child;
+	int port1;
+
+	usb_hub_for_each_child(udev, port1, child) {
+		if (usb_sideband_check(child))
+			return true;
+	}
+
+	return !!refcount_read(&udev->sb_usage_count);
+}
+EXPORT_SYMBOL_GPL(usb_sideband_check);
+
 /**
  * usb_autosuspend_device - delayed autosuspend of a USB device and its interfaces
  * @udev: the usb_device to autosuspend
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 672d8fc2abdb..37a36750a851 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -645,6 +645,7 @@ struct usb3_lpm_parameters {
  *	parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns)
  *	Will be used as wValue for SetIsochDelay requests.
  * @use_generic_driver: ask driver core to reprobe using the generic driver.
+ * @sb_usage_count: number of active sideband accessing this usb device.
  *
  * Notes:
  * Usbcore drivers should not set usbdev->state directly.  Instead use
@@ -731,6 +732,8 @@ struct usb_device {
 
 	u16 hub_delay;
 	unsigned use_generic_driver:1;
+
+	refcount_t sb_usage_count;
 };
 
 #define to_usb_device(__dev)	container_of_const(__dev, struct usb_device, dev)
@@ -798,6 +801,9 @@ static inline int usb_acpi_port_lpm_incapable(struct usb_device *hdev, int index
 #ifdef CONFIG_PM
 extern void usb_enable_autosuspend(struct usb_device *udev);
 extern void usb_disable_autosuspend(struct usb_device *udev);
+extern void usb_sideband_get(struct usb_device *udev);
+extern void usb_sideband_put(struct usb_device *udev);
+extern bool usb_sideband_check(struct usb_device *udev);
 
 extern int usb_autopm_get_interface(struct usb_interface *intf);
 extern void usb_autopm_put_interface(struct usb_interface *intf);
@@ -818,6 +824,13 @@ static inline int usb_enable_autosuspend(struct usb_device *udev)
 static inline int usb_disable_autosuspend(struct usb_device *udev)
 { return 0; }
 
+static inline int usb_sideband_get(struct usb_device *udev)
+{ return 0; }
+static inline int usb_sideband_put(struct usb_device *udev)
+{ return 0; }
+static inline bool usb_sideband_check(struct usb_device *udev)
+{ return false; }
+
 static inline int usb_autopm_get_interface(struct usb_interface *intf)
 { return 0; }
 static inline int usb_autopm_get_interface_async(struct usb_interface *intf)
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v5 4/5] xhci: sideband: add api to trace sideband usage

[Guan-Yu Lin]

The existing sideband driver only registers sidebands without tracking
their active usage. To address this, new apis are introduced to:
- mark sideband usage: record the sideband usage information in the USB
  host controller driver and USB device driver.
- query sideband status: provide a means for other drivers to fetch
  sideband activity information on a USB host controller.

Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
 drivers/usb/host/xhci-sideband.c  | 74 +++++++++++++++++++++++++++++++
 include/linux/usb/hcd.h           |  4 ++
 include/linux/usb/xhci-sideband.h |  5 +++
 3 files changed, 83 insertions(+)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index d04cf0af57ae..87dd66056324 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -334,6 +334,80 @@ xhci_sideband_interrupter_id(struct xhci_sideband *sb)
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_interrupter_id);
 
+/**
+ * xhci_sideband_get - inform related drivers there's a new active sideband
+ * @sb: sideband instance for this usb device
+ *
+ * An active sideband indicates that another entity is currently using the host
+ * controller. Inform the host controller and related usb devices by increasing
+ * their sb_usage_count. This allows the corresponding drivers to dynamically
+ * adjust power management actions based on current sideband activity.
+ *
+ * Returns 0 on success, negative error otherwise
+ */
+int xhci_sideband_get(struct xhci_sideband *sb)
+{
+	struct usb_hcd *hcd;
+	struct usb_device *udev;
+
+	if (!sb || !sb->xhci)
+		return -ENODEV;
+
+	hcd = xhci_to_hcd(sb->xhci);
+	refcount_inc(&hcd->sb_usage_count);
+
+	udev = sb->vdev->udev;
+	usb_sideband_get(udev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_sideband_get);
+
+/**
+ * xhci_sideband_put - inform related drivers there's a sideband deactivated
+ * @sb: sideband instance for this usb device
+ *
+ * The inverse operation of xhci_sideband_get, which informs the host
+ * controller and related usb devices by decreasing their sb_usage_count. This
+ * allows the corresponding drivers to dynamically adjust power management
+ * actions based on current sideband activity.
+ *
+ * Returns 0 on success, negative error otherwise
+ */
+int xhci_sideband_put(struct xhci_sideband *sb)
+{
+	struct usb_hcd *hcd;
+	struct usb_device *udev;
+
+	if (!sb || !sb->xhci)
+		return -ENODEV;
+
+	hcd = xhci_to_hcd(sb->xhci);
+	refcount_dec(&hcd->sb_usage_count);
+
+	udev = sb->vdev->udev;
+	usb_sideband_put(udev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_sideband_put);
+
+/**
+ * xhci_sideband_check - check sideband activities on the host controller
+ * @hcd: the host controller driver associated with the target host controller
+ *
+ * Allow other drivers, such as usb controller driver, to check if there are
+ * any sideband activity on the host controller right now. This information
+ * could be used for power management or other forms or resource management.
+ *
+ * Returns true on any active sideband existence, false otherwise
+ */
+bool xhci_sideband_check(struct usb_hcd *hcd)
+{
+	return !!refcount_read(&hcd->sb_usage_count);
+}
+EXPORT_SYMBOL_GPL(xhci_sideband_check);
+
 /**
  * xhci_sideband_register - register a sideband for a usb device
  * @udev: usb device to be accessed via sideband
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index ac95e7c89df5..d4f5e57b0c00 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -84,6 +84,10 @@ struct usb_hcd {
 	struct urb		*status_urb;	/* the current status urb */
 #ifdef CONFIG_PM
 	struct work_struct	wakeup_work;	/* for remote wakeup */
+#ifdef CONFIG_USB_XHCI_SIDEBAND
+	/* Number of active sideband accessing the host controller. */
+	refcount_t		sb_usage_count;
+#endif
 #endif
 	struct work_struct	died_work;	/* for when the device dies */
 
diff --git a/include/linux/usb/xhci-sideband.h b/include/linux/usb/xhci-sideband.h
index f0223c5535e0..4850fc826e00 100644
--- a/include/linux/usb/xhci-sideband.h
+++ b/include/linux/usb/xhci-sideband.h
@@ -12,6 +12,7 @@
 
 #include <linux/scatterlist.h>
 #include <linux/usb.h>
+#include <linux/usb/hcd.h>
 
 #define	EP_CTX_PER_DEV		31	/* FIXME defined twice, from xhci.h */
 
@@ -57,6 +58,10 @@ xhci_sideband_get_endpoint_buffer(struct xhci_sideband *sb,
 struct sg_table *
 xhci_sideband_get_event_buffer(struct xhci_sideband *sb);
 
+int xhci_sideband_get(struct xhci_sideband *sb);
+int xhci_sideband_put(struct xhci_sideband *sb);
+bool xhci_sideband_check(struct usb_hcd *hcd);
+
 int
 xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 				 bool ip_autoclear, u32 imod_interval, int intr_num);
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Guan-Yu Lin]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Greg KH]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Mathias Nyman]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Alan Stern]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Guan-Yu Lin]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Guan-Yu Lin]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Guan-Yu Lin]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Alan Stern]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Guan-Yu Lin]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Alan Stern]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Guan-Yu Lin]

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

Re: [PATCH v5 5/5] usb: host: enable sideband transfer during system sleep

[Alan Stern]

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