[PATCH v2 0/2] add hooks for usb suspend and resume

[PATCH v2 0/2] add hooks for usb suspend and resume

[Puma Hsu]

In mobile, a co-processor can be used for USB audio. When the co-processor
is working for USB audio, the co-processor is the user/owner of the USB
driver, and the ACPU is able to sleep in such condition to improve power
consumption. In order to support this, we need to create hooks in suspend
and resume functions. We also upload our implementations for reviewing.

Puma Hsu (2):
  usb: core: add hooks for usb suspend and resume
  usb: core: add implementations for usb suspend/resume hooks

 drivers/usb/core/driver.c              | 36 +++++++++++++
 drivers/usb/core/usb-hooks-impl-goog.c | 72 ++++++++++++++++++++++++++
 drivers/usb/core/usb.h                 |  5 ++
 3 files changed, 113 insertions(+)
 create mode 100644 drivers/usb/core/usb-hooks-impl-goog.c

---
Changes in v2:
- Remove the wrong input in the Makefile
- Change description in commit message

-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 1/2] usb: core: add hooks for usb suspend and resume

[Puma Hsu]

Add the hooks that designer can design and bypass the original
suspend/resume. When the handled is set, skip the original
suspend/resume process.

In mobile, a co-processor can be used for USB audio. When the co-processor
is working for USB audio, the co-processor is the user/owner of the USB
driver, and the ACPU is able to sleep in such condition to improve power
consumption. In original process, the ACPU will suspend/resume until the
USB suspend/resume. We add the hooks, so we can control USB suspend/resume
without affecting the ACPU.

Signed-off-by: Puma Hsu <pumahsu@google.com>

---
Changes in v2:
- Remove the wrong input in the Makefile
- Change description in commit message

---
 drivers/usb/core/driver.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/usb/core/usb.h    |  5 +++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 7e7e119c253f..3d2cfb6c2277 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -35,6 +35,25 @@
 #include "usb.h"
 
 
+static struct usb_device_vendor_ops *usb_dev_vendor_ops;
+
+int usb_dev_register_vendor_ops(struct usb_device_vendor_ops *vendor_ops)
+{
+	if (vendor_ops == NULL)
+		return -EINVAL;
+
+	usb_dev_vendor_ops = vendor_ops;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_dev_register_vendor_ops);
+
+struct usb_device_vendor_ops *usb_vendor_get_ops(void)
+{
+	return usb_dev_vendor_ops;
+}
+EXPORT_SYMBOL_GPL(usb_vendor_get_ops);
+
+
 /*
  * Adds a new dynamic USBdevice ID to this driver,
  * and cause the driver to probe for all devices again.
@@ -1400,11 +1419,19 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
 	int			status = 0;
 	int			i = 0, n = 0;
 	struct usb_interface	*intf;
+	bool			handled;
+	struct usb_device_vendor_ops *ops = usb_vendor_get_ops();
 
 	if (udev->state == USB_STATE_NOTATTACHED ||
 			udev->state == USB_STATE_SUSPENDED)
 		goto done;
 
+	if (ops && ops->usb_dev_suspend) {
+		handled = ops->usb_dev_suspend(udev, msg);
+		if (handled)
+			goto done;
+	}
+
 	/* Suspend all the interfaces and then udev itself */
 	if (udev->actconfig) {
 		n = udev->actconfig->desc.bNumInterfaces;
@@ -1501,11 +1528,20 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
 	int			status = 0;
 	int			i;
 	struct usb_interface	*intf;
+	bool			handled;
+	struct usb_device_vendor_ops *ops = usb_vendor_get_ops();
 
 	if (udev->state == USB_STATE_NOTATTACHED) {
 		status = -ENODEV;
 		goto done;
 	}
+
+	if (ops && ops->usb_dev_resume) {
+		handled = ops->usb_dev_resume(udev, msg);
+		if (handled)
+			goto done;
+	}
+
 	udev->can_submit = 1;
 
 	/* Resume the device */
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..9ccb8683071d 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -220,3 +220,8 @@ extern acpi_handle usb_get_hub_port_acpi_handle(struct usb_device *hdev,
 static inline int usb_acpi_register(void) { return 0; };
 static inline void usb_acpi_unregister(void) { };
 #endif
+
+struct usb_device_vendor_ops {
+	bool (*usb_dev_suspend)(struct usb_device *udev, pm_message_t msg);
+	bool (*usb_dev_resume)(struct usb_device *udev, pm_message_t msg);
+};
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 2/2] usb: core: add implementations for usb suspend/resume hooks

[Puma Hsu]

In mobile, a co-processor can be used for USB audio. When the co-processor
is working for USB audio, the co-processor is the user/owner of the USB
driver, and the ACPU is able to sleep in such condition to improve power
consumption. In order to support this, we implement the hooks to handle USB
suspend/resume requests.

This commit introduces two hook implementations:
- usb_device_vendor_suspend()
  Determine whether we should skip suspend request according to the status
  of USB audio playback/capture.
  Return:
  - true: let driver.c know that we "handled" and it can just return
          succeeded to ACPU to continue system suspend process.
  - false: let driver.c know that it still run original suspend process.

- usb_device_vendor_resume()
  Determine whether we should skip resume request according to the USB
  device's suspend state.
  Return:
  - true: let driver.c know that it doesn't need to run resume process.
  - false: let driver.c know that it still run original resume process.

Signed-off-by: Puma Hsu <pumahsu@google.com>
---
 drivers/usb/core/usb-hooks-impl-goog.c | 72 ++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 drivers/usb/core/usb-hooks-impl-goog.c

diff --git a/drivers/usb/core/usb-hooks-impl-goog.c b/drivers/usb/core/usb-hooks-impl-goog.c
new file mode 100644
index 000000000000..89dc360babed
--- /dev/null
+++ b/drivers/usb/core/usb-hooks-impl-goog.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Google Corp.
+ *
+ * Author:
+ *  Puma Hsu <pumahsu@google.com>
+ */
+
+#include <linux/usb.h>
+#include "usb.h"
+
+extern int usb_dev_register_vendor_ops(struct usb_device_vendor_ops *vendor_ops);
+
+static bool usb_device_vendor_suspend(struct usb_device *udev, pm_message_t msg)
+{
+	bool usb_playback = false;
+	bool usb_capture = false;
+	bool handled = false;
+
+	if (!udev)
+		return handled;
+
+	/*
+	 * Note: Our private driver provides APIs to know the device is in audio playback
+	 * or capture.
+	 *
+	 * usb_playback = usb_audio_playback_enabled();
+	 * usb_capture = usb_audio_capture_enabled();
+	 */
+
+	/*
+	 * Note: When the USB audio is working, we will not let the usb device suspend.
+	 * Return handled = true so that the System core can it's suspend process.
+	 */
+	if (usb_playback || usb_capture) {
+		dev_info(&udev->dev, "%s: skip suspend process (playback:%d,capture:%d)\n",
+			 __func__, usb_playback, usb_capture);
+		handled = true;
+	}
+
+	return handled;
+}
+
+static bool usb_device_vendor_resume(struct usb_device *udev, pm_message_t msg)
+{
+	bool handled = false;
+
+	if (!udev)
+		return handled;
+
+	/*
+	 * Note: If the udev didn't suspend actually, we don't need to do resume.
+	 */
+	if (udev->port_is_suspended || udev->state == USB_STATE_SUSPENDED) {
+		handled = false;
+	} else {
+		dev_info(&udev->dev, "%s: skip resume process\n", __func__);
+		handled = true;
+	}
+
+	return handled;
+}
+
+static struct usb_device_vendor_ops usb_dev_vendor_ops = {
+	.usb_dev_suspend = usb_device_vendor_suspend,
+	.usb_dev_resume = usb_device_vendor_resume,
+};
+
+int usb_vendor_helper_init(void)
+{
+	return usb_dev_register_vendor_ops(&usb_dev_vendor_ops);
+}
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v2 2/2] usb: core: add implementations for usb suspend/resume hooks

[Christoph Hellwig]

Again, this is never actually registered.  It it not tied to an device.
What is the point of all this?  Are you trying to fool us?

Re: [PATCH v2 0/2] add hooks for usb suspend and resume

[Greg KH]

On Wed, Dec 14, 2022 at 04:14:54PM +0800, Puma Hsu wrote:
> In mobile, a co-processor can be used for USB audio. When the co-processor
> is working for USB audio, the co-processor is the user/owner of the USB
> driver, and the ACPU is able to sleep in such condition to improve power
> consumption. In order to support this, we need to create hooks in suspend
> and resume functions. We also upload our implementations for reviewing.
> 
> Puma Hsu (2):
>   usb: core: add hooks for usb suspend and resume
>   usb: core: add implementations for usb suspend/resume hooks
> 
>  drivers/usb/core/driver.c              | 36 +++++++++++++
>  drivers/usb/core/usb-hooks-impl-goog.c | 72 ++++++++++++++++++++++++++
>  drivers/usb/core/usb.h                 |  5 ++
>  3 files changed, 113 insertions(+)
>  create mode 100644 drivers/usb/core/usb-hooks-impl-goog.c
> 
> ---
> Changes in v2:
> - Remove the wrong input in the Makefile
> - Change description in commit message
> 
> -- 
> 2.39.0.rc1.256.g54fd8350bd-goog
> 

My review comments on v1 still pertain here.

Re: [PATCH v2 0/2] add hooks for usb suspend and resume

[Oliver Neukum]

On 14.12.22 09:14, Puma Hsu wrote:
> In mobile, a co-processor can be used for USB audio. When the co-processor
> is working for USB audio, the co-processor is the user/owner of the USB
> driver, and the ACPU is able to sleep in such condition to improve power
> consumption. In order to support this, we need to create hooks in suspend
> and resume functions. We also upload our implementations for reviewing.

Ok, before this gets hopelessly unproductive, please describe what
you are aiming at and operating on in greater detail.

It looks to me like you have an audio device that is connected
to the host by USB _and_ another bus. Is that correct?
Will you submit the "subdriver" that drives the device over
that secondary bus?
The operation over the secondary bus requires a hook in USB
power management. Why?

Secondly, the naming is atrocious.

Now could we please first define the technical nature of the issue,
so that we fully understand it before we debate the desirability?

	Regards
		Oliver

Re: [PATCH v2 0/2] add hooks for usb suspend and resume

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Fri, Dec 23, 2022 at 11:00:00AM +0800, Puma Hsu wrote:
> Thanks for the comments. I will have an introduction for our design first.

<snip>

Please put this in the next round of patches you all submit, and work
together to NOT submit independant patch series when they are obviously
interconnected.

thanks,

greg k-h