* [PATCH v4 1/5] usb: dwc3: separate dev_pm_ops for each pm_event
2024-10-09 5:42 [PATCH v4 0/5] Support system sleep with offloaded usb transfers Guan-Yu Lin
@ 2024-10-09 5:42 ` Guan-Yu Lin
2024-10-09 12:45 ` Greg KH
2024-10-09 5:42 ` [PATCH v4 2/5] usb: xhci-plat: " Guan-Yu Lin
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-09 5:42 UTC (permalink / raw)
To: Thinh.Nguyen, gregkh, mathias.nyman, stern, elder, oneukum,
yajun.deng, dianders, kekrby, perex, tiwai, tj, stanley_chang,
andreyknvl, christophe.jaillet, quic_jjohnson, ricardo, grundler,
niko.mauno
Cc: linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu, 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/5] usb: dwc3: separate dev_pm_ops for each pm_event
2024-10-09 5:42 ` [PATCH v4 1/5] usb: dwc3: separate dev_pm_ops for each pm_event Guan-Yu Lin
@ 2024-10-09 12:45 ` Greg KH
2024-10-10 4:12 ` Guan-Yu Lin
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2024-10-09 12:45 UTC (permalink / raw)
To: Guan-Yu Lin
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 1/5] usb: dwc3: separate dev_pm_ops for each pm_event
2024-10-09 12:45 ` Greg KH
@ 2024-10-10 4:12 ` Guan-Yu Lin
0 siblings, 0 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-10 4:12 UTC (permalink / raw)
To: Greg KH
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 2/5] usb: xhci-plat: separate dev_pm_ops for each pm_event
2024-10-09 5:42 [PATCH v4 0/5] Support system sleep with offloaded usb transfers Guan-Yu Lin
2024-10-09 5:42 ` [PATCH v4 1/5] usb: dwc3: separate dev_pm_ops for each pm_event Guan-Yu Lin
@ 2024-10-09 5:42 ` Guan-Yu Lin
2024-10-09 5:42 ` [PATCH v4 3/5] usb: add apis for sideband uasge tracking Guan-Yu Lin
` (2 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-09 5:42 UTC (permalink / raw)
To: Thinh.Nguyen, gregkh, mathias.nyman, stern, elder, oneukum,
yajun.deng, dianders, kekrby, perex, tiwai, tj, stanley_chang,
andreyknvl, christophe.jaillet, quic_jjohnson, ricardo, grundler,
niko.mauno
Cc: linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu, 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-09 5:42 [PATCH v4 0/5] Support system sleep with offloaded usb transfers Guan-Yu Lin
2024-10-09 5:42 ` [PATCH v4 1/5] usb: dwc3: separate dev_pm_ops for each pm_event Guan-Yu Lin
2024-10-09 5:42 ` [PATCH v4 2/5] usb: xhci-plat: " Guan-Yu Lin
@ 2024-10-09 5:42 ` Guan-Yu Lin
2024-10-09 7:33 ` Amadeusz Sławiński
2024-10-09 12:44 ` Greg KH
2024-10-09 5:42 ` [PATCH v4 4/5] xhci: sideband: add api to trace sideband usage Guan-Yu Lin
2024-10-09 5:42 ` [PATCH v4 5/5] usb: host: enable sideband transfer during system sleep Guan-Yu Lin
4 siblings, 2 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-09 5:42 UTC (permalink / raw)
To: Thinh.Nguyen, gregkh, mathias.nyman, stern, elder, oneukum,
yajun.deng, dianders, kekrby, perex, tiwai, tj, stanley_chang,
andreyknvl, christophe.jaillet, quic_jjohnson, ricardo, grundler,
niko.mauno
Cc: linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu, 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-09 5:42 ` [PATCH v4 3/5] usb: add apis for sideband uasge tracking Guan-Yu Lin
@ 2024-10-09 7:33 ` Amadeusz Sławiński
2024-10-09 11:36 ` Guan-Yu Lin
2024-10-09 12:44 ` Greg KH
1 sibling, 1 reply; 20+ messages in thread
From: Amadeusz Sławiński @ 2024-10-09 7:33 UTC (permalink / raw)
To: Guan-Yu Lin, Thinh.Nguyen, gregkh, mathias.nyman, stern, elder,
oneukum, yajun.deng, dianders, kekrby, perex, tiwai, tj,
stanley_chang, andreyknvl, christophe.jaillet, quic_jjohnson,
ricardo, grundler, niko.mauno
Cc: linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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);
> +
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-09 7:33 ` Amadeusz Sławiński
@ 2024-10-09 11:36 ` Guan-Yu Lin
0 siblings, 0 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-09 11:36 UTC (permalink / raw)
To: Amadeusz Sławiński
Cc: Thinh.Nguyen, gregkh, mathias.nyman, stern, elder, oneukum,
yajun.deng, dianders, kekrby, perex, tiwai, tj, stanley_chang,
andreyknvl, christophe.jaillet, quic_jjohnson, ricardo, grundler,
niko.mauno, linux-usb, linux-kernel, linux-sound, badhri,
albertccwang, quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-09 5:42 ` [PATCH v4 3/5] usb: add apis for sideband uasge tracking Guan-Yu Lin
2024-10-09 7:33 ` Amadeusz Sławiński
@ 2024-10-09 12:44 ` Greg KH
2024-10-10 5:30 ` Guan-Yu Lin
1 sibling, 1 reply; 20+ messages in thread
From: Greg KH @ 2024-10-09 12:44 UTC (permalink / raw)
To: Guan-Yu Lin
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-09 12:44 ` Greg KH
@ 2024-10-10 5:30 ` Guan-Yu Lin
2024-10-10 6:33 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-10 5:30 UTC (permalink / raw)
To: Greg KH
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-10 5:30 ` Guan-Yu Lin
@ 2024-10-10 6:33 ` Greg KH
2024-10-10 16:14 ` Guan-Yu Lin
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2024-10-10 6:33 UTC (permalink / raw)
To: Guan-Yu Lin
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-10 6:33 ` Greg KH
@ 2024-10-10 16:14 ` Guan-Yu Lin
2024-10-11 4:30 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-10 16:14 UTC (permalink / raw)
To: Greg KH
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-10 16:14 ` Guan-Yu Lin
@ 2024-10-11 4:30 ` Greg KH
2024-10-11 7:33 ` Guan-Yu Lin
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2024-10-11 4:30 UTC (permalink / raw)
To: Guan-Yu Lin
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking
2024-10-11 4:30 ` Greg KH
@ 2024-10-11 7:33 ` Guan-Yu Lin
0 siblings, 0 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-11 7:33 UTC (permalink / raw)
To: Greg KH
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 4/5] xhci: sideband: add api to trace sideband usage
2024-10-09 5:42 [PATCH v4 0/5] Support system sleep with offloaded usb transfers Guan-Yu Lin
` (2 preceding siblings ...)
2024-10-09 5:42 ` [PATCH v4 3/5] usb: add apis for sideband uasge tracking Guan-Yu Lin
@ 2024-10-09 5:42 ` Guan-Yu Lin
2024-10-09 12:47 ` Greg KH
2024-10-09 5:42 ` [PATCH v4 5/5] usb: host: enable sideband transfer during system sleep Guan-Yu Lin
4 siblings, 1 reply; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-09 5:42 UTC (permalink / raw)
To: Thinh.Nguyen, gregkh, mathias.nyman, stern, elder, oneukum,
yajun.deng, dianders, kekrby, perex, tiwai, tj, stanley_chang,
andreyknvl, christophe.jaillet, quic_jjohnson, ricardo, grundler,
niko.mauno
Cc: linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu, 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/5] xhci: sideband: add api to trace sideband usage
2024-10-09 5:42 ` [PATCH v4 4/5] xhci: sideband: add api to trace sideband usage Guan-Yu Lin
@ 2024-10-09 12:47 ` Greg KH
2024-10-10 4:16 ` Guan-Yu Lin
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2024-10-09 12:47 UTC (permalink / raw)
To: Guan-Yu Lin
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 4/5] xhci: sideband: add api to trace sideband usage
2024-10-09 12:47 ` Greg KH
@ 2024-10-10 4:16 ` Guan-Yu Lin
0 siblings, 0 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-10 4:16 UTC (permalink / raw)
To: Greg KH
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 5/5] usb: host: enable sideband transfer during system sleep
2024-10-09 5:42 [PATCH v4 0/5] Support system sleep with offloaded usb transfers Guan-Yu Lin
` (3 preceding siblings ...)
2024-10-09 5:42 ` [PATCH v4 4/5] xhci: sideband: add api to trace sideband usage Guan-Yu Lin
@ 2024-10-09 5:42 ` Guan-Yu Lin
2024-10-09 12:47 ` Greg KH
4 siblings, 1 reply; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-09 5:42 UTC (permalink / raw)
To: Thinh.Nguyen, gregkh, mathias.nyman, stern, elder, oneukum,
yajun.deng, dianders, kekrby, perex, tiwai, tj, stanley_chang,
andreyknvl, christophe.jaillet, quic_jjohnson, ricardo, grundler,
niko.mauno
Cc: linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu, 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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 5/5] usb: host: enable sideband transfer during system sleep
2024-10-09 5:42 ` [PATCH v4 5/5] usb: host: enable sideband transfer during system sleep Guan-Yu Lin
@ 2024-10-09 12:47 ` Greg KH
2024-10-11 7:34 ` Guan-Yu Lin
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2024-10-09 12:47 UTC (permalink / raw)
To: Guan-Yu Lin
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 5/5] usb: host: enable sideband transfer during system sleep
2024-10-09 12:47 ` Greg KH
@ 2024-10-11 7:34 ` Guan-Yu Lin
0 siblings, 0 replies; 20+ messages in thread
From: Guan-Yu Lin @ 2024-10-11 7:34 UTC (permalink / raw)
To: Greg KH
Cc: Thinh.Nguyen, mathias.nyman, stern, elder, oneukum, yajun.deng,
dianders, kekrby, perex, tiwai, tj, stanley_chang, andreyknvl,
christophe.jaillet, quic_jjohnson, ricardo, grundler, niko.mauno,
linux-usb, linux-kernel, linux-sound, badhri, albertccwang,
quic_wcheng, pumahsu
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
^ permalink raw reply [flat|nested] 20+ messages in thread