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

[PATCH v4 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 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 uasge tracking
  xhci: sideband: add api to trace sideband usage
  usb: host: enable sideband transfer during system sleep

 drivers/usb/core/driver.c         | 64 ++++++++++++++++++++++
 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, 303 insertions(+), 5 deletions(-)

-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH v4 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.rc0.187.ge670bccf7e-goog

[PATCH v4 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.rc0.187.ge670bccf7e-goog

[PATCH v4 3/5] usb: add apis for sideband uasge 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 | 54 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h       | 13 ++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 0c3f12daac79..c1ba5ed15214 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1673,6 +1673,60 @@ void usb_disable_autosuspend(struct usb_device *udev)
 }
 EXPORT_SYMBOL_GPL(usb_disable_autosuspend);
 
+/**
+ * usb_sideband_get - notify usb driver there's a new active sideband
+ * @udev: the usb_device which has an active sideband
+ *
+ * An active sideband indicates that another entity is currently using the usb
+ * device. Notify the usb device by increasing the sb_usage_count. This allows
+ * usb driver to adjust power management policy based on sideband activities.
+ */
+void usb_sideband_get(struct usb_device *udev)
+{
+	struct usb_device *parent = udev;
+
+	do {
+		atomic_inc(&parent->sb_usage_count);
+		parent = parent->parent;
+	} while (parent);
+}
+EXPORT_SYMBOL_GPL(usb_sideband_get);
+
+/**
+ * usb_sideband_put - notify usb driver there's a sideband deactivated
+ * @udev: the usb_device which has a sideband deactivated
+ *
+ * The inverse operation of usb_sideband_get, which notifies the usb device by
+ * decreasing the sb_usage_count. This allows usb driver to adjust power
+ * management policy based on sideband activities.
+ */
+void usb_sideband_put(struct usb_device *udev)
+{
+	struct usb_device *parent = udev;
+
+	do {
+		atomic_dec(&parent->sb_usage_count);
+		parent = parent->parent;
+	} while (parent);
+}
+EXPORT_SYMBOL_GPL(usb_sideband_put);
+
+/**
+ * usb_sideband_check - check sideband activities on the host controller
+ * @udev: the usb_device which has a sideband deactivated
+ *
+ * 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)
+{
+	return !!atomic_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..5b9fea378960 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;
+
+	atomic_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.rc0.187.ge670bccf7e-goog

[PATCH v4 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..29501e5d020f 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 - notify 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. Notify 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);
+	atomic_inc(&hcd->sb_usage_count);
+
+	udev = sb->vdev->udev;
+	usb_sideband_get(udev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_sideband_get);
+
+/**
+ * xhci_sideband_put - notify related drivers there's a sideband deactivated
+ * @sb: sideband instance for this usb device
+ *
+ * The inverse operation of xhci_sideband_get, which notifies 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);
+	atomic_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 !!atomic_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..8b8106b9a31e 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. */
+	atomic_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.rc0.187.ge670bccf7e-goog

[PATCH v4 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 c1ba5ed15214..83726bf88fb6 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_info(dev, "device active, skip %s", __func__);
+		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_info(dev, "device active, skip %s", __func__);
+		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..a41f1fa425bd 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);
+	atomic_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..69fbbc6f865f 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);
+	atomic_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..d1ad817ff564 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_info(dev, "device active, skip %s", __func__);
+		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_info(dev, "device active, skip %s", __func__);
+		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..b730320df70d 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_info(dev, "device active, skip %s", __func__);
+		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_info(dev, "device active, skip %s", __func__);
+		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.rc0.187.ge670bccf7e-goog

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Amadeusz Sławiński]

On 10/9/2024 7:42 AM, Guan-Yu Lin wrote:
> 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 | 54 +++++++++++++++++++++++++++++++++++++++
>   include/linux/usb.h       | 13 ++++++++++
>   2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 0c3f12daac79..c1ba5ed15214 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1673,6 +1673,60 @@ void usb_disable_autosuspend(struct usb_device *udev)
>   }
>   EXPORT_SYMBOL_GPL(usb_disable_autosuspend);
>   
> +/**
> + * usb_sideband_get - notify usb driver there's a new active sideband
> + * @udev: the usb_device which has an active sideband
> + *
> + * An active sideband indicates that another entity is currently using the usb
> + * device. Notify the usb device by increasing the sb_usage_count. This allows
> + * usb driver to adjust power management policy based on sideband activities.
> + */
> +void usb_sideband_get(struct usb_device *udev)
> +{
> +	struct usb_device *parent = udev;

Is it really "parent" in this case? Perhaps better variable name would 
just be "device".

> +
> +	do {
> +		atomic_inc(&parent->sb_usage_count);
> +		parent = parent->parent;
> +	} while (parent);
> +}
> +EXPORT_SYMBOL_GPL(usb_sideband_get);
> +
> +/**
> + * usb_sideband_put - notify usb driver there's a sideband deactivated
> + * @udev: the usb_device which has a sideband deactivated
> + *
> + * The inverse operation of usb_sideband_get, which notifies the usb device by
> + * decreasing the sb_usage_count. This allows usb driver to adjust power
> + * management policy based on sideband activities.
> + */
> +void usb_sideband_put(struct usb_device *udev)
> +{
> +	struct usb_device *parent = udev;

Similarly here.

> +
> +	do {
> +		atomic_dec(&parent->sb_usage_count);
> +		parent = parent->parent;
> +	} while (parent);
> +}
> +EXPORT_SYMBOL_GPL(usb_sideband_put);
> +

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Guan-Yu Lin]

On Wed, Oct 9, 2024 at 3:33 PM Amadeusz Sławiński
<amadeuszx.slawinski@linux.intel.com> wrote:
>
> On 10/9/2024 7:42 AM, Guan-Yu Lin wrote:
> >
> > +void usb_sideband_get(struct usb_device *udev)
> > +{
> > +     struct usb_device *parent = udev;
>
> Is it really "parent" in this case? Perhaps better variable name would
> just be "device".
>
Thanks for the heads-up, will change it to "device" in the next patchset.
> > +
> > +     do {
> > +             atomic_inc(&parent->sb_usage_count);
> > +             parent = parent->parent;
> > +     } while (parent);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_sideband_get);
> > +
>
> Similarly here.
>
> > +
> > +     do {
> > +             atomic_dec(&parent->sb_usage_count);
> > +             parent = parent->parent;
> > +     } while (parent);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_sideband_put);
> > +

Regards,
Guan-Yu

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Greg KH]

On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> 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 | 54 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h       | 13 ++++++++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 0c3f12daac79..c1ba5ed15214 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1673,6 +1673,60 @@ void usb_disable_autosuspend(struct usb_device *udev)
>  }
>  EXPORT_SYMBOL_GPL(usb_disable_autosuspend);
>  
> +/**
> + * usb_sideband_get - notify usb driver there's a new active sideband
> + * @udev: the usb_device which has an active sideband
> + *
> + * An active sideband indicates that another entity is currently using the usb
> + * device. Notify the usb device by increasing the sb_usage_count. This allows
> + * usb driver to adjust power management policy based on sideband activities.
> + */
> +void usb_sideband_get(struct usb_device *udev)
> +{
> +	struct usb_device *parent = udev;
> +
> +	do {
> +		atomic_inc(&parent->sb_usage_count);

As this is a reference count, use refcount_t please.

> +		parent = parent->parent;
> +	} while (parent);

Woah, walking up the device chain?  That should not be needed, or if so,
then each device's "usage count" is pointless.

> +}
> +EXPORT_SYMBOL_GPL(usb_sideband_get);
> +
> +/**
> + * usb_sideband_put - notify usb driver there's a sideband deactivated
> + * @udev: the usb_device which has a sideband deactivated
> + *
> + * The inverse operation of usb_sideband_get, which notifies the usb device by
> + * decreasing the sb_usage_count. This allows usb driver to adjust power
> + * management policy based on sideband activities.
> + */
> +void usb_sideband_put(struct usb_device *udev)
> +{
> +	struct usb_device *parent = udev;
> +
> +	do {
> +		atomic_dec(&parent->sb_usage_count);
> +		parent = parent->parent;
> +	} while (parent);
> +}
> +EXPORT_SYMBOL_GPL(usb_sideband_put);
> +
> +/**
> + * usb_sideband_check - check sideband activities on the host controller
> + * @udev: the usb_device which has a sideband deactivated
> + *
> + * 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)
> +{
> +	return !!atomic_read(&udev->sb_usage_count);

And what happens if it changes right after you make this call?  This
feels racy and broken.

> +}
> +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..5b9fea378960 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;
> +
> +	atomic_t sb_usage_count;

Why is this on the device and not the interface that is bound to the
driver that is doing this work?

thanks,

greg k-h

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

[Greg KH]

On Wed, Oct 09, 2024 at 05:42:55AM +0000, Guan-Yu Lin wrote:
> 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);

Why is power off hibernate?

This needs an ack from the dwc3 maintainer as I can't determine if it's
correct at all...

thanks,

greg k-h

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

[Greg KH]

On Wed, Oct 09, 2024 at 05:42:59AM +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 c1ba5ed15214..83726bf88fb6 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_info(dev, "device active, skip %s", __func__);

When drivers work properly, they are quiet.  Why all of the loud
shouting in this patch as it goes about it's business?

also, __func__ is redundant in dev_*() calls :)

thanks,

greg k-h

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

[Greg KH]

On Wed, Oct 09, 2024 at 05:42:58AM +0000, Guan-Yu Lin wrote:
> --- 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. */
> +	atomic_t		sb_usage_count;

It's a reference count, use refcount_t please.

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

[Guan-Yu Lin]

On Wed, Oct 9, 2024 at 8:45 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 09, 2024 at 05:42:55AM +0000, Guan-Yu Lin wrote:
> > +
> > +static int dwc3_poweroff(struct device *dev)
> > +{
> > +     struct dwc3     *dwc = dev_get_drvdata(dev);
> > +     int             ret;
> > +
> > +     ret = dwc3_suspend_common(dwc, PMSG_HIBERNATE);
>
> Why is power off hibernate?
>
> This needs an ack from the dwc3 maintainer as I can't determine if it's
> correct at all...
>
> thanks,
>
> greg k-h

Described in /include/linux/pm.h, PM_EVENT_HIBERNATE message denotes
the following transition in the PM core code:
"Hibernation image has been saved, call ->prepare() and ->poweroff()
for all devices."
Meantime, the interpretation of the the above description could be
found in /drivers/base/power/main.c:
static pm_callback_t pm_op(const struct dev_pm_ops *ops, pm_message_t state)
{
...
        case PM_EVENT_HIBERNATE:
                return ops->poweroff;
...
}
An example in device drivers could be found in usb/drivers/usb/core/usb.c:
static int usb_dev_suspend(struct device *dev)
{
        return usb_suspend(dev, PMSG_SUSPEND);
}

Regards,
Guan-Yu

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

[Guan-Yu Lin]

On Wed, Oct 9, 2024 at 8:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 09, 2024 at 05:42:58AM +0000, Guan-Yu Lin wrote:
> > --- 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. */
> > +     atomic_t                sb_usage_count;
>
> It's a reference count, use refcount_t please.

Appreciate the clear instructions, let me fix it in the next version.

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Guan-Yu Lin]

On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> > +void usb_sideband_get(struct usb_device *udev)
> > +{
> > +     struct usb_device *parent = udev;
> > +
> > +     do {
> > +             atomic_inc(&parent->sb_usage_count);
>
> As this is a reference count, use refcount_t please.

Acknowledged, will change it in the next patch. Thanks for the guidance.

>
> > +             parent = parent->parent;
> > +     } while (parent);
>
> Woah, walking up the device chain?  That should not be needed, or if so,
> then each device's "usage count" is pointless.
>

Say a hub X with usb devices A,B,C attached on it, where usb device A
is actively used by sideband now. We'd like to introduce a mechanism
so that hub X won't have to iterate through all its children to
determine sideband activities under this usb device tree. This problem
is similar to runtime suspending a device, where rpm uses
power.usage_count for tracking activity of the device itself and
power.child_count to check the children's activity. In our scenario,
we don't see the need to separate activities on the device itself or
on its children. So we combine two counters in rpm as sb_usage_count,
denoting the sideband activities under a specific usb device. We have
to keep a counter in each device so that we won't influence the usb
devices that aren't controlled by a sideband.
When sideband activity changes on a usb device, its usb device parents
should all get notified to maintain the correctness of sb_usage_count.
This notifying process creates the procedure to walk up the device
chain.

> > +bool usb_sideband_check(struct usb_device *udev)
> > +{
> > +     return !!atomic_read(&udev->sb_usage_count);
>
> And what happens if it changes right after you make this call?  This
> feels racy and broken.
>

Seems like we need a mechanism to block any new sideband access after
the usb device has been suspended. How about adding a lock during the
period when the usb device is suspended? Do you think this is the
correct direction to address the race condition?

> > @@ -731,6 +732,8 @@ struct usb_device {
> >
> >       u16 hub_delay;
> >       unsigned use_generic_driver:1;
> > +
> > +     atomic_t sb_usage_count;
>
> Why is this on the device and not the interface that is bound to the
> driver that is doing this work?
>
> thanks,
>
> greg k-h

If the count is bound on the usb interface, I'm afraid that the
sideband information couldn't be broadcasted across its usb device
parents. Do you have some insight on how we can achieve this?

Regards,
Guan-Yu

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Greg KH]

On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote:
> On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> > > +void usb_sideband_get(struct usb_device *udev)
> > > +{
> > > +     struct usb_device *parent = udev;
> > > +
> > > +     do {
> > > +             atomic_inc(&parent->sb_usage_count);
> >
> > As this is a reference count, use refcount_t please.
> 
> Acknowledged, will change it in the next patch. Thanks for the guidance.
> 
> >
> > > +             parent = parent->parent;
> > > +     } while (parent);
> >
> > Woah, walking up the device chain?  That should not be needed, or if so,
> > then each device's "usage count" is pointless.
> >
> 
> Say a hub X with usb devices A,B,C attached on it, where usb device A
> is actively used by sideband now. We'd like to introduce a mechanism
> so that hub X won't have to iterate through all its children to
> determine sideband activities under this usb device tree.

Why would a hub care?

> This problem
> is similar to runtime suspending a device, where rpm uses
> power.usage_count for tracking activity of the device itself and
> power.child_count to check the children's activity. In our scenario,
> we don't see the need to separate activities on the device itself or
> on its children.

But that's exactly what is needed here, if a hub wants to know what is
happening on a child device, it should just walk the list of children
and look :)

> So we combine two counters in rpm as sb_usage_count,

Combining counters is almost always never a good idea and will come back
to bite you in the end.  Memory isn't an issue here, speed isn't an
issue here, so why not just do it properly?

> denoting the sideband activities under a specific usb device. We have
> to keep a counter in each device so that we won't influence the usb
> devices that aren't controlled by a sideband.

I can understand that for the device being "controlled" by a sideband,
but that's it.

> When sideband activity changes on a usb device, its usb device parents
> should all get notified to maintain the correctness of sb_usage_count.

Why "should" they?  They shouldn't really care.

> This notifying process creates the procedure to walk up the device
> chain.

You aren't notifying anyone, you are just incrementing a count that can
change at any moment in time.

> > > +bool usb_sideband_check(struct usb_device *udev)
> > > +{
> > > +     return !!atomic_read(&udev->sb_usage_count);
> >
> > And what happens if it changes right after you make this call?  This
> > feels racy and broken.
> >
> 
> Seems like we need a mechanism to block any new sideband access after
> the usb device has been suspended. How about adding a lock during the
> period when the usb device is suspended? Do you think this is the
> correct direction to address the race condition?

I don't know, as I don't know exactly what you are going to do with this
information.  But as-is, you can't just go "let's put an atomic variable
in there to make it race free" as that's just not going to work.

> > > @@ -731,6 +732,8 @@ struct usb_device {
> > >
> > >       u16 hub_delay;
> > >       unsigned use_generic_driver:1;
> > > +
> > > +     atomic_t sb_usage_count;
> >
> > Why is this on the device and not the interface that is bound to the
> > driver that is doing this work?
> >
> > thanks,
> >
> > greg k-h
> 
> If the count is bound on the usb interface, I'm afraid that the
> sideband information couldn't be broadcasted across its usb device
> parents. Do you have some insight on how we can achieve this?

But the driver that is "sideband" is bound to the interface, not the
device, right?  Or is it the device?

And again, nothing is being "broadcasted" here, it's just a variable to
be read at random times and can change randomly :)

thanks,

greg k-h

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Guan-Yu Lin]

On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote:
> > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> > > > +             parent = parent->parent;
> > > > +     } while (parent);
> > >
> > > Woah, walking up the device chain?  That should not be needed, or if so,
> > > then each device's "usage count" is pointless.
> > >
> >
> > Say a hub X with usb devices A,B,C attached on it, where usb device A
> > is actively used by sideband now. We'd like to introduce a mechanism
> > so that hub X won't have to iterate through all its children to
> > determine sideband activities under this usb device tree.
>
> Why would a hub care?
>

Without the information of sideband activities on the usb devices
connected to the hub, the hub couldn't determine if it could suspend
or not.

> > This problem
> > is similar to runtime suspending a device, where rpm uses
> > power.usage_count for tracking activity of the device itself and
> > power.child_count to check the children's activity. In our scenario,
> > we don't see the need to separate activities on the device itself or
> > on its children.
>
> But that's exactly what is needed here, if a hub wants to know what is
> happening on a child device, it should just walk the list of children
> and look :)
>
> > So we combine two counters in rpm as sb_usage_count,
>
> Combining counters is almost always never a good idea and will come back
> to bite you in the end.  Memory isn't an issue here, speed isn't an
> issue here, so why not just do it properly?
>

By combining the two comments above, my understanding is that we should either:
1. separating the counter to one recording the sideband activity of
itself, one for its children.
2. walk the list of children to check sideband activities on demand.
Please correct me if I mistake your messages.

> > denoting the sideband activities under a specific usb device. We have
> > to keep a counter in each device so that we won't influence the usb
> > devices that aren't controlled by a sideband.
>
> I can understand that for the device being "controlled" by a sideband,
> but that's it.
>
> > When sideband activity changes on a usb device, its usb device parents
> > should all get notified to maintain the correctness of sb_usage_count.
>
> Why "should" they?  They shouldn't really care.
>

Hubs need the sideband activity information on downstream usb devices,
so that the hub won't suspend the upstream usb port when there is a
sideband accessing the usb device connected to it.

> > This notifying process creates the procedure to walk up the device
> > chain.
>
> You aren't notifying anyone, you are just incrementing a count that can
> change at any moment in time.
>

Apologies for the misleading word selection, by notify I mean changing
the counter.

> > > > +bool usb_sideband_check(struct usb_device *udev)
> > > > +{
> > > > +     return !!atomic_read(&udev->sb_usage_count);
> > >
> > > And what happens if it changes right after you make this call?  This
> > > feels racy and broken.
> > >
> >
> > Seems like we need a mechanism to block any new sideband access after
> > the usb device has been suspended. How about adding a lock during the
> > period when the usb device is suspended? Do you think this is the
> > correct direction to address the race condition?
>
> I don't know, as I don't know exactly what you are going to do with this
> information.  But as-is, you can't just go "let's put an atomic variable
> in there to make it race free" as that's just not going to work.
>

Agree that changing the variable to atomic wouldn't resolve race
conditions. Let me identify and mark critical sections in the next
patchset.

> > > > @@ -731,6 +732,8 @@ struct usb_device {
> > > >
> > > >       u16 hub_delay;
> > > >       unsigned use_generic_driver:1;
> > > > +
> > > > +     atomic_t sb_usage_count;
> > >
> > > Why is this on the device and not the interface that is bound to the
> > > driver that is doing this work?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > If the count is bound on the usb interface, I'm afraid that the
> > sideband information couldn't be broadcasted across its usb device
> > parents. Do you have some insight on how we can achieve this?
>
> But the driver that is "sideband" is bound to the interface, not the
> device, right?  Or is it the device?
>
> And again, nothing is being "broadcasted" here, it's just a variable to
> be read at random times and can change randomly :)
>
> thanks,
>
> greg k-h

By looking at the comment in xhci-sideband.c, I think the sideband is
bound to an usb device instead of an usb interface. Maybe Mathias or
Wesley could provide more details on this.
/**
 * xhci_sideband_register - register a sideband for a usb device
 * @udev: usb device to be accessed via sideband
...
 */

Regards,
Guan-Yu

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Greg KH]

On Fri, Oct 11, 2024 at 12:14:00AM +0800, Guan-Yu Lin wrote:
> On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote:
> > > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> > > > > +             parent = parent->parent;
> > > > > +     } while (parent);
> > > >
> > > > Woah, walking up the device chain?  That should not be needed, or if so,
> > > > then each device's "usage count" is pointless.
> > > >
> > >
> > > Say a hub X with usb devices A,B,C attached on it, where usb device A
> > > is actively used by sideband now. We'd like to introduce a mechanism
> > > so that hub X won't have to iterate through all its children to
> > > determine sideband activities under this usb device tree.
> >
> > Why would a hub care?
> >
> 
> Without the information of sideband activities on the usb devices
> connected to the hub, the hub couldn't determine if it could suspend
> or not.

You are talking about an "internal" hub, right?  And isn't this already
covered by the original sideband patchset?  If not, how is power
management being handled there?

> > > This problem
> > > is similar to runtime suspending a device, where rpm uses
> > > power.usage_count for tracking activity of the device itself and
> > > power.child_count to check the children's activity. In our scenario,
> > > we don't see the need to separate activities on the device itself or
> > > on its children.
> >
> > But that's exactly what is needed here, if a hub wants to know what is
> > happening on a child device, it should just walk the list of children
> > and look :)
> >
> > > So we combine two counters in rpm as sb_usage_count,
> >
> > Combining counters is almost always never a good idea and will come back
> > to bite you in the end.  Memory isn't an issue here, speed isn't an
> > issue here, so why not just do it properly?
> >
> 
> By combining the two comments above, my understanding is that we should either:
> 1. separating the counter to one recording the sideband activity of
> itself, one for its children.
> 2. walk the list of children to check sideband activities on demand.
> Please correct me if I mistake your messages.

I think 2 is better, as this is infrequent and should be pretty fast to
do when needed, right?  But I really don't care, just don't combine
things together that shouldn't be combined.

> > > denoting the sideband activities under a specific usb device. We have
> > > to keep a counter in each device so that we won't influence the usb
> > > devices that aren't controlled by a sideband.
> >
> > I can understand that for the device being "controlled" by a sideband,
> > but that's it.
> >
> > > When sideband activity changes on a usb device, its usb device parents
> > > should all get notified to maintain the correctness of sb_usage_count.
> >
> > Why "should" they?  They shouldn't really care.
> >
> 
> Hubs need the sideband activity information on downstream usb devices,
> so that the hub won't suspend the upstream usb port when there is a
> sideband accessing the usb device connected to it.

Then why doesn't the sideband code just properly mark the "downstream"
device a being used like any other normal device?  Why is this acting
"special"?

thanks,

greg k-h

Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Guan-Yu Lin]

On Fri, Oct 11, 2024 at 12:40 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 11, 2024 at 12:14:00AM +0800, Guan-Yu Lin wrote:
> > On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote:
> > > > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> > > > > > +             parent = parent->parent;
> > > > > > +     } while (parent);
> > > > >
> > > > > Woah, walking up the device chain?  That should not be needed, or if so,
> > > > > then each device's "usage count" is pointless.
> > > > >
> > > >
> > > > Say a hub X with usb devices A,B,C attached on it, where usb device A
> > > > is actively used by sideband now. We'd like to introduce a mechanism
> > > > so that hub X won't have to iterate through all its children to
> > > > determine sideband activities under this usb device tree.
> > >
> > > Why would a hub care?
> > >
> >
> > Without the information of sideband activities on the usb devices
> > connected to the hub, the hub couldn't determine if it could suspend
> > or not.
>
> You are talking about an "internal" hub, right?  And isn't this already
> covered by the original sideband patchset?  If not, how is power
> management being handled there?
>

I'm referring to both internal and external hubs. As a sideband is
designed to handle the transfers on specific endpoints, I think
there's a possibility the usb device controlled by the sideband is
connected to the host controller by a hierarchy of usb hubs. The
current proposal of sideband, AFAIK, only supports sideband accessing
usb devices when the system is active, whereas now we're proposing
patchset to enable sideband access when the system is in sleep.

> > > > This problem
> > > > is similar to runtime suspending a device, where rpm uses
> > > > power.usage_count for tracking activity of the device itself and
> > > > power.child_count to check the children's activity. In our scenario,
> > > > we don't see the need to separate activities on the device itself or
> > > > on its children.
> > >
> > > But that's exactly what is needed here, if a hub wants to know what is
> > > happening on a child device, it should just walk the list of children
> > > and look :)
> > >
> > > > So we combine two counters in rpm as sb_usage_count,
> > >
> > > Combining counters is almost always never a good idea and will come back
> > > to bite you in the end.  Memory isn't an issue here, speed isn't an
> > > issue here, so why not just do it properly?
> > >
> >
> > By combining the two comments above, my understanding is that we should either:
> > 1. separating the counter to one recording the sideband activity of
> > itself, one for its children.
> > 2. walk the list of children to check sideband activities on demand.
> > Please correct me if I mistake your messages.
>
> I think 2 is better, as this is infrequent and should be pretty fast to
> do when needed, right?  But I really don't care, just don't combine
> things together that shouldn't be combined.
>

Thanks for the clarification, I'll renew the patchset based on the discussions.

> > > > denoting the sideband activities under a specific usb device. We have
> > > > to keep a counter in each device so that we won't influence the usb
> > > > devices that aren't controlled by a sideband.
> > >
> > > I can understand that for the device being "controlled" by a sideband,
> > > but that's it.
> > >
> > > > When sideband activity changes on a usb device, its usb device parents
> > > > should all get notified to maintain the correctness of sb_usage_count.
> > >
> > > Why "should" they?  They shouldn't really care.
> > >
> >
> > Hubs need the sideband activity information on downstream usb devices,
> > so that the hub won't suspend the upstream usb port when there is a
> > sideband accessing the usb device connected to it.
>
> Then why doesn't the sideband code just properly mark the "downstream"
> device a being used like any other normal device?  Why is this acting
> "special"?
>
> thanks,
>
> greg k-h

In runtime power management, sidebands could mark a device active by
runtime pm apis as we usually did. However, there will be
ambiguity when we're doing device power management in system suspend.
A usb device being marked as runtime active could be either:
1) actively function through the existing usb driver stacks.
2) accessed by a sideband, where the usb driver stacks in the linux
system aren't involved.
In 1) system should suspend the devices, ports, controllers as normal
because usb transfers are also suspended during system suspend. On the
other hand, in 2) we should keep the necessary device, port,
controller active to support sideband access during system suspend.
Hence, we need the sideband access information of each usb_device
during system power state transition.

Regards,
Guan-Yu

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

[Guan-Yu Lin]

On Wed, Oct 9, 2024 at 8:47 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 09, 2024 at 05:42:59AM +0000, Guan-Yu Lin wrote:
> > @@ -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_info(dev, "device active, skip %s", __func__);
>
> When drivers work properly, they are quiet.  Why all of the loud
> shouting in this patch as it goes about it's business?
>
> also, __func__ is redundant in dev_*() calls :)
>
> thanks,
>
> greg k-h

Thanks for the suggestions. Let me switch to dev_dbg and remove
__func__ in the next patch.

Regards,
Guan-Yu