| Message ID | 20241014085816.1401364-6-guanyulin@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support system sleep with offloaded usb transfers | expand |
On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote: > Sharing a USB controller with another entity via xhci-sideband driver > creates power management complexities. To prevent the USB controller > from being inadvertently deactivated while in use by the other entity, a > usage-count based mechanism is implemented. This allows the system to > manage power effectively, ensuring the controller remains available > whenever needed. > > Signed-off-by: Guan-Yu Lin <guanyulin@google.com> > --- > drivers/usb/core/driver.c | 10 ++++++++++ > drivers/usb/core/hcd.c | 1 + > drivers/usb/core/usb.c | 1 + > drivers/usb/dwc3/core.c | 13 +++++++++++++ > drivers/usb/dwc3/core.h | 8 ++++++++ > drivers/usb/host/xhci-plat.c | 10 ++++++++++ > drivers/usb/host/xhci-plat.h | 7 +++++++ > sound/usb/qcom/qc_audio_offload.c | 3 +++ > 8 files changed, 53 insertions(+) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index e713cf9b3dd2..eb85cbb1a2ff 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int r; > > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { > + dev_dbg(dev, "device accessed via sideband\n"); > + return 0; > + } What prevents the check from changing state right after you call this? > + > unbind_no_pm_drivers_interfaces(udev); > > /* From now on we are sure all drivers support suspend/resume > @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int status; > > + if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) { > + dev_dbg(dev, "device accessed via sideband\n"); > + return 0; > + } Same here, what's keeping the state from changing? > + > /* For all calls, take the device back to full power and > * tell the PM core in case it was autosuspended previously. > * Unbind the interfaces that will need rebinding later, > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 1ff7d901fede..9876b3940281 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, > timer_setup(&hcd->rh_timer, rh_timer_func, 0); > #ifdef CONFIG_PM > INIT_WORK(&hcd->wakeup_work, hcd_resume_work); > + refcount_set(&hcd->sb_usage_count, 0); > #endif > > INIT_WORK(&hcd->died_work, hcd_died_work); > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 0b4685aad2d5..d315d066a56b 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent, > dev->state = USB_STATE_ATTACHED; > dev->lpm_disable_count = 1; > atomic_set(&dev->urbnum, 0); > + refcount_set(&dev->sb_usage_count, 0); > > INIT_LIST_HEAD(&dev->ep0.urb_list); > dev->ep0.desc.bLength = USB_DT_ENDPOINT_SIZE; > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 2fdafbcbe44c..18c743ce5ac5 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -2550,8 +2550,15 @@ static int dwc3_runtime_idle(struct device *dev) > static int dwc3_suspend(struct device *dev) > { > struct dwc3 *dwc = dev_get_drvdata(dev); > + struct platform_device *xhci = dwc->xhci; > int ret; > > + if (xhci && xhci_sideband_check(xhci->dev.driver_data)) { What could go wrong with poking into a random device structure's private data that you don't know the type of? :( > + dev_dbg(dev, "device accessed via sideband\n"); > + return 0; I predict, that if this all does get implemented, we're going to have a lot of confusion of "why will my devices not go into suspend?" questions, right? How does userspace know if a device is controlled by a sideband path or not? Is there some sysfs link somewhere, and does any tool show it anyway? thanks, greg k-h
On 14.10.2024 11.50, Guan-Yu Lin wrote: > Sharing a USB controller with another entity via xhci-sideband driver > creates power management complexities. To prevent the USB controller > from being inadvertently deactivated while in use by the other entity, a > usage-count based mechanism is implemented. This allows the system to > manage power effectively, ensuring the controller remains available > whenever needed. I don't think all this is needed to prevent USB controller from being deactivated while sideband is in use. The modified audio class driver that uses sideband is still bound to a usb interface device, and all normal pm reference counting should work. To me it looks like this code is tricking pm framework into believing the usb device and host controller have successfully suspended during system suspend when they in reality are fully up and running. I'm not sure I fully understand the case this series is solving. Thanks Mathias
On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote: > Sharing a USB controller with another entity via xhci-sideband driver > creates power management complexities. To prevent the USB controller > from being inadvertently deactivated while in use by the other entity, a > usage-count based mechanism is implemented. This allows the system to > manage power effectively, ensuring the controller remains available > whenever needed. > > Signed-off-by: Guan-Yu Lin <guanyulin@google.com> > --- > drivers/usb/core/driver.c | 10 ++++++++++ > drivers/usb/core/hcd.c | 1 + > drivers/usb/core/usb.c | 1 + > drivers/usb/dwc3/core.c | 13 +++++++++++++ > drivers/usb/dwc3/core.h | 8 ++++++++ > drivers/usb/host/xhci-plat.c | 10 ++++++++++ > drivers/usb/host/xhci-plat.h | 7 +++++++ > sound/usb/qcom/qc_audio_offload.c | 3 +++ > 8 files changed, 53 insertions(+) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index e713cf9b3dd2..eb85cbb1a2ff 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int r; > > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { > + dev_dbg(dev, "device accessed via sideband\n"); > + return 0; > + } I'm not so sure about this. By returning early, you prevent the drivers bound to this device from suspending. But they can't operate properly when the system is in a low-power mode. Won't that cause problems? Maybe this really belongs in usb_suspend_device(), and its counterpart belongs in usb_resume_device(). > + > unbind_no_pm_drivers_interfaces(udev); > > /* From now on we are sure all drivers support suspend/resume > @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int status; > > + if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) { > + dev_dbg(dev, "device accessed via sideband\n"); > + return 0; > + } > + > /* For all calls, take the device back to full power and > * tell the PM core in case it was autosuspended previously. > * Unbind the interfaces that will need rebinding later, > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 1ff7d901fede..9876b3940281 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, > timer_setup(&hcd->rh_timer, rh_timer_func, 0); > #ifdef CONFIG_PM > INIT_WORK(&hcd->wakeup_work, hcd_resume_work); > + refcount_set(&hcd->sb_usage_count, 0); Did I miss something? I didn't notice this field in any of the earlier patches. Was it already created by the prerequisite series? If so, why didn't that series do this initialization? > #endif > > INIT_WORK(&hcd->died_work, hcd_died_work); > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 0b4685aad2d5..d315d066a56b 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent, > dev->state = USB_STATE_ATTACHED; > dev->lpm_disable_count = 1; > atomic_set(&dev->urbnum, 0); > + refcount_set(&dev->sb_usage_count, 0); And doesn't this belong in the 3/5 patch, the one that creates the sb_usage_count field? Alan Stern
On Mon, Oct 14, 2024 at 11:56:40AM -0400, Alan Stern wrote: > On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote: > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > > index 1ff7d901fede..9876b3940281 100644 > > --- a/drivers/usb/core/hcd.c > > +++ b/drivers/usb/core/hcd.c > > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, > > timer_setup(&hcd->rh_timer, rh_timer_func, 0); > > #ifdef CONFIG_PM > > INIT_WORK(&hcd->wakeup_work, hcd_resume_work); > > + refcount_set(&hcd->sb_usage_count, 0); > > Did I miss something? I didn't notice this field in any of the earlier > patches. Was it already created by the prerequisite series? If so, why > didn't that series do this initialization? Oops, yes, I did miss it -- it's in the 4/5 patch. Sorry. Alan Stern
On Mon, Oct 14, 2024 at 5:21 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote: > > > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > > index e713cf9b3dd2..eb85cbb1a2ff 100644 > > --- a/drivers/usb/core/driver.c > > +++ b/drivers/usb/core/driver.c > > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) > > struct usb_device *udev = to_usb_device(dev); > > int r; > > > > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { > > + dev_dbg(dev, "device accessed via sideband\n"); > > + return 0; > > + } > > What prevents the check from changing state right after you call this? > Maybe we should hold a lock before doing the check, and release the lock after the entire system suspend period to ensure no sideband will access the USB device after it has been suspended. However, I'm not very confident about holding a lock over the entire suspend period. Should we do so or use a boolean flag instead? I'm not sure if holding a lock will prevent the system from entering low-power state or not. > > + > > unbind_no_pm_drivers_interfaces(udev); > > > > /* From now on we are sure all drivers support suspend/resume > > @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg) > > struct usb_device *udev = to_usb_device(dev); > > int status; > > > > + if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) { > > + dev_dbg(dev, "device accessed via sideband\n"); > > + return 0; > > + } > > Same here, what's keeping the state from changing? > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 2fdafbcbe44c..18c743ce5ac5 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -2550,8 +2550,15 @@ static int dwc3_runtime_idle(struct device *dev) > > static int dwc3_suspend(struct device *dev) > > { > > struct dwc3 *dwc = dev_get_drvdata(dev); > > + struct platform_device *xhci = dwc->xhci; > > int ret; > > > > + if (xhci && xhci_sideband_check(xhci->dev.driver_data)) { > > What could go wrong with poking into a random device structure's private > data that you don't know the type of? :( > We did observe that the driver_data was set to struct usb_hcd in the "platform:xhci-hcd" module, which was the default linux xhci host controller platform glue driver. Should we rewrite as "struct usb_hcd *hcd = dev_get_drvdata(dev);" and then operate with hcd? Or even with this kind of statement the idea is still unacceptable? > > + dev_dbg(dev, "device accessed via sideband\n"); > > + return 0; > > I predict, that if this all does get implemented, we're going to have a > lot of confusion of "why will my devices not go into suspend?" > questions, right? > This feature should not influence anyone not using xhci sideband framework. For those who use xhci sideband framework, the current design seems to not support USB transfer during system suspend. I think the goal for this patch is to enable system suspend while holding necessary devices active to enable USB transfer via sideband, and suspend all devices if we don't have USB transfers on it. The USB devices are active only when there are USB transfers happening on it. Based on this, I think users will have less concern on why the USB devices are still active. > How does userspace know if a device is controlled by a sideband path or > not? Is there some sysfs link somewhere, and does any tool show it > anyway? > > thanks, > > greg k-h I don't recall there's a mechanism to show sideband activity to userspace. Is this need solely related to enabling USB transfer via sideband during system suspend? Or is it related to generic sideband functionality? If the latter is the case, maybe we should propose a patch in series "Introduce QC USB SND audio offloading support" to support this mechanism. Regards, Guan-Yu
On Mon, Oct 14, 2024 at 9:06 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 14.10.2024 11.50, Guan-Yu Lin wrote: > > Sharing a USB controller with another entity via xhci-sideband driver > > creates power management complexities. To prevent the USB controller > > from being inadvertently deactivated while in use by the other entity, a > > usage-count based mechanism is implemented. This allows the system to > > manage power effectively, ensuring the controller remains available > > whenever needed. > > I don't think all this is needed to prevent USB controller from being > deactivated while sideband is in use. The modified audio class driver > that uses sideband is still bound to a usb interface device, and all > normal pm reference counting should work. > > To me it looks like this code is tricking pm framework into believing > the usb device and host controller have successfully suspended during > system suspend when they in reality are fully up and running. > > I'm not sure I fully understand the case this series is solving. > > Thanks > Mathias > Your understanding is correct; this series aims to keep necessary devices running while the system is suspended. The goal is to keep USB transfer available via sideband when the main system is asleep. Since we're offloading some USB transfers to another audio DSP, the Linux driver is not involved in queueing transfer TRBs, handling event TRBs, etc., in some specific "offloaded" USB endpoints. The sideband might be bound to a USB interface device to prevent the device from going into runtime suspend, but it seems this doesn't prevent devices from suspending when the system goes into system-wide suspend. So the main purpose of this series is to prevent some related devices being suspended during system suspend. Regards, Guan-Yu
On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote: > > > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > > index e713cf9b3dd2..eb85cbb1a2ff 100644 > > --- a/drivers/usb/core/driver.c > > +++ b/drivers/usb/core/driver.c > > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) > > struct usb_device *udev = to_usb_device(dev); > > int r; > > > > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { > > + dev_dbg(dev, "device accessed via sideband\n"); > > + return 0; > > + } > > I'm not so sure about this. By returning early, you prevent the drivers > bound to this device from suspending. But they can't operate properly > when the system is in a low-power mode. Won't that cause problems? > > Maybe this really belongs in usb_suspend_device(), and its counterpart > belongs in usb_resume_device(). > To my understanding, after the system is suspended, the USB driver will do nothing as the main processor has been suspended. May I check what forms of low power mode and operation we are discussing here? usb_suspend_device() did close the required port/bus to maintain usb transfer. However, there are additional usb_hcd_flush_endpoint() calls aside from usb_suspend_device(). Maybe we should consider not doing those also since some of the endpoints are now handled by the sideband. > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > > index 1ff7d901fede..9876b3940281 100644 > > --- a/drivers/usb/core/hcd.c > > +++ b/drivers/usb/core/hcd.c > > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, > > timer_setup(&hcd->rh_timer, rh_timer_func, 0); > > #ifdef CONFIG_PM > > INIT_WORK(&hcd->wakeup_work, hcd_resume_work); > > + refcount_set(&hcd->sb_usage_count, 0); > > Did I miss something? I didn't notice this field in any of the earlier > patches. Was it already created by the prerequisite series? If so, why > didn't that series do this initialization? > > > #endif > > > > INIT_WORK(&hcd->died_work, hcd_died_work); > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > index 0b4685aad2d5..d315d066a56b 100644 > > --- a/drivers/usb/core/usb.c > > +++ b/drivers/usb/core/usb.c > > @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent, > > dev->state = USB_STATE_ATTACHED; > > dev->lpm_disable_count = 1; > > atomic_set(&dev->urbnum, 0); > > + refcount_set(&dev->sb_usage_count, 0); > > And doesn't this belong in the 3/5 patch, the one that creates the > sb_usage_count field? > > Alan Stern Thanks for the suggestion, I'll move this, as well as the initialization of hcd->sb_usage_count, to corresponding earlier patches. Regards, Guan-Yu
On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote: > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > I'm not so sure about this. By returning early, you prevent the drivers > > bound to this device from suspending. But they can't operate properly > > when the system is in a low-power mode. Won't that cause problems? > > > > Maybe this really belongs in usb_suspend_device(), and its counterpart > > belongs in usb_resume_device(). > > > > To my understanding, after the system is suspended, the USB driver > will do nothing as the main processor has been suspended. May I check > what forms of low power mode and operation we are discussing here? S3 suspend. You are right that the driver will do nothing while the CPU is suspended. But what about the times before and after that, while the suspend and resume procedures are underway? The driver needs to be told to cancel any ongoing transfers while the system suspends and then restart them while the system resumes. > usb_suspend_device() did close the required port/bus to maintain usb > transfer. I don't know what you mean by that. > However, there are additional usb_hcd_flush_endpoint() calls > aside from usb_suspend_device(). Maybe we should consider not doing > those also since some of the endpoints are now handled by the > sideband. Those calls should not be skipped. If the sideband controller wants to handle the endpoints on its own, it can go right ahead. The main controller and the USB core need to know that they shouldn't use the endpoint while the system is suspending. Alan Stern
On Tue, Oct 15, 2024 at 10:43 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote: > > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > I'm not so sure about this. By returning early, you prevent the drivers > > > bound to this device from suspending. But they can't operate properly > > > when the system is in a low-power mode. Won't that cause problems? > > > > > > Maybe this really belongs in usb_suspend_device(), and its counterpart > > > belongs in usb_resume_device(). > > > > > > > To my understanding, after the system is suspended, the USB driver > > will do nothing as the main processor has been suspended. May I check > > what forms of low power mode and operation we are discussing here? > > S3 suspend. You are right that the driver will do nothing while the > CPU is suspended. But what about the times before and after that, > while the suspend and resume procedures are underway? The driver > needs to be told to cancel any ongoing transfers while the system > suspends and then restart them while the system resumes. > Regarding the cancellation of ongoing transfers during suspend, I believe usb_hcd_flush_endpoint() handles this as discussed below. Besides calling usb_hcd_flush_endpoint(), are there any other necessary changes before suspending the driver in our scenario? Maybe we could discuss setting usb_device_state to USB_STATE_SUSPENDED. However, my understanding is that this variable reflects the actual device state. Since the device remains active via the sideband in our case, changing usb_device_state seems unnecessary. > > usb_suspend_device() did close the required port/bus to maintain usb > > transfer. > > I don't know what you mean by that. > Nothing special here, I'm just echoing what you've mentioned and trying to bring up the discussion about usb_hcd_flush_endpoint(). > > However, there are additional usb_hcd_flush_endpoint() calls > > aside from usb_suspend_device(). Maybe we should consider not doing > > those also since some of the endpoints are now handled by the > > sideband. > > Those calls should not be skipped. If the sideband controller wants > to handle the endpoints on its own, it can go right ahead. The main > controller and the USB core need to know that they shouldn't use the > endpoint while the system is suspending. > > Alan Stern Got it, let me update the patch and put the related changes into usb_suspend_device()/usb_resume_device(). Regards, Guan-Yu
On Wed, Oct 16, 2024 at 03:40:00PM +0800, Guan-Yu Lin wrote: > On Tue, Oct 15, 2024 at 10:43 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote: > > > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > I'm not so sure about this. By returning early, you prevent the drivers > > > > bound to this device from suspending. But they can't operate properly > > > > when the system is in a low-power mode. Won't that cause problems? > > > > > > > > Maybe this really belongs in usb_suspend_device(), and its counterpart > > > > belongs in usb_resume_device(). > > > > > > > > > > To my understanding, after the system is suspended, the USB driver > > > will do nothing as the main processor has been suspended. May I check > > > what forms of low power mode and operation we are discussing here? > > > > S3 suspend. You are right that the driver will do nothing while the > > CPU is suspended. But what about the times before and after that, > > while the suspend and resume procedures are underway? The driver > > needs to be told to cancel any ongoing transfers while the system > > suspends and then restart them while the system resumes. > > > > Regarding the cancellation of ongoing transfers during suspend, I > believe usb_hcd_flush_endpoint() handles this as discussed below. There's more to it than that. If you cancel ongoing transfers by calling usb_hcd_flush_endpoint() without informing the driver first, the driver will get very confused and think the device has failed. > Besides calling usb_hcd_flush_endpoint(), are there any other > necessary changes before suspending the driver in our scenario? Maybe > we could discuss setting usb_device_state to USB_STATE_SUSPENDED. > However, my understanding is that this variable reflects the actual > device state. Since the device remains active via the sideband in our > case, changing usb_device_state seems unnecessary. That's right. I don't think anything else is needed. Just call usb_suspend_interface() like the normal pathway in usb_suspend_both() does, but skip calling usb_suspend_device(). Alan Stern
On Wed, Oct 16, 2024 at 10:45 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Oct 16, 2024 at 03:40:00PM +0800, Guan-Yu Lin wrote: > > On Tue, Oct 15, 2024 at 10:43 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Tue, Oct 15, 2024 at 11:56:00AM +0800, Guan-Yu Lin wrote: > > > > On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > I'm not so sure about this. By returning early, you prevent the drivers > > > > > bound to this device from suspending. But they can't operate properly > > > > > when the system is in a low-power mode. Won't that cause problems? > > > > > > > > > > Maybe this really belongs in usb_suspend_device(), and its counterpart > > > > > belongs in usb_resume_device(). > > > > > > > > > > > > > To my understanding, after the system is suspended, the USB driver > > > > will do nothing as the main processor has been suspended. May I check > > > > what forms of low power mode and operation we are discussing here? > > > > > > S3 suspend. You are right that the driver will do nothing while the > > > CPU is suspended. But what about the times before and after that, > > > while the suspend and resume procedures are underway? The driver > > > needs to be told to cancel any ongoing transfers while the system > > > suspends and then restart them while the system resumes. > > > > > > > Regarding the cancellation of ongoing transfers during suspend, I > > believe usb_hcd_flush_endpoint() handles this as discussed below. > > There's more to it than that. If you cancel ongoing transfers by > calling usb_hcd_flush_endpoint() without informing the driver first, the > driver will get very confused and think the device has failed. > > > Besides calling usb_hcd_flush_endpoint(), are there any other > > necessary changes before suspending the driver in our scenario? Maybe > > we could discuss setting usb_device_state to USB_STATE_SUSPENDED. > > However, my understanding is that this variable reflects the actual > > device state. Since the device remains active via the sideband in our > > case, changing usb_device_state seems unnecessary. > > That's right. > > I don't think anything else is needed. Just call > usb_suspend_interface() like the normal pathway in usb_suspend_both() > does, but skip calling usb_suspend_device(). > > Alan Stern Thanks for the suggestions, let me address them in the next version. After some local development, our experiments suggest it may be necessary to skip usb_suspend_interface() & usb_hcd_flush_endpoint() for connection changes behind a hub and HID events in our scenario. Typically, when the system sleeps, the hub uses remote wakeup to reactivate upstream devices and resume the interface to handle connection changes. However, our current conclusion is to maintain the device in an active state while suspending the interface. This deviates from the norm, as remote wakeup is designed to function when devices and links are suspended. We're concerned that this discrepancy might interfere with the remote wakeup mechanism. To address this, we're currently bypassing usb_suspend_interface() and usb_hcd_flush_endpoint(). This effectively simulates an "active system" state, allowing the USB controller to notify the kernel about connection changes via interrupts. This workaround applies to HID events as well. Which approach do you recommend? Should we invest in integrating with the remote wakeup framework, or is it acceptable to keep necessary components active, mirroring an "active system" state? Regards, Guan-Yu
On Fri, Oct 18, 2024 at 07:59:00PM +0800, Guan-Yu Lin wrote: > Thanks for the suggestions, let me address them in the next version. > After some local development, our experiments suggest it may be > necessary to skip usb_suspend_interface() & usb_hcd_flush_endpoint() > for connection changes behind a hub and HID events in our scenario. > > Typically, when the system sleeps, the hub uses remote wakeup to > reactivate upstream devices and resume the interface to handle > connection changes. However, our current conclusion is to maintain the > device in an active state while suspending the interface. This > deviates from the norm, as remote wakeup is designed to function when > devices and links are suspended. We're concerned that this discrepancy > might interfere with the remote wakeup mechanism. > To address this, we're currently bypassing usb_suspend_interface() and > usb_hcd_flush_endpoint(). This effectively simulates an "active > system" state, allowing the USB controller to notify the kernel about > connection changes via interrupts. This workaround applies to HID > events as well. > > Which approach do you recommend? Should we invest in integrating with > the remote wakeup framework, or is it acceptable to keep necessary > components active, mirroring an "active system" state? It's hard to answer those questions because I don't have a clear idea of how the sideband system is meant to work. For instance, what does the sideband system do when a port-connect change is detected in a hub that sits between the computer and the audio device? If the sideband system decides to change the audio device's settings, how does the regular audio driver learn about the change? And so on. It's worth pointing out that allowing two different drivers to control the same device is generally not a good idea. More likely than not they will end up interfering with each other at some stage. Alan Stern
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index e713cf9b3dd2..eb85cbb1a2ff 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) struct usb_device *udev = to_usb_device(dev); int r; + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { + dev_dbg(dev, "device accessed via sideband\n"); + return 0; + } + unbind_no_pm_drivers_interfaces(udev); /* From now on we are sure all drivers support suspend/resume @@ -1619,6 +1624,11 @@ int usb_resume(struct device *dev, pm_message_t msg) struct usb_device *udev = to_usb_device(dev); int status; + if (msg.event == PM_EVENT_RESUME && usb_sideband_check(udev)) { + dev_dbg(dev, "device accessed via sideband\n"); + return 0; + } + /* For all calls, take the device back to full power and * tell the PM core in case it was autosuspended previously. * Unbind the interfaces that will need rebinding later, diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 1ff7d901fede..9876b3940281 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, timer_setup(&hcd->rh_timer, rh_timer_func, 0); #ifdef CONFIG_PM INIT_WORK(&hcd->wakeup_work, hcd_resume_work); + refcount_set(&hcd->sb_usage_count, 0); #endif INIT_WORK(&hcd->died_work, hcd_died_work); diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 0b4685aad2d5..d315d066a56b 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent, dev->state = USB_STATE_ATTACHED; dev->lpm_disable_count = 1; atomic_set(&dev->urbnum, 0); + refcount_set(&dev->sb_usage_count, 0); INIT_LIST_HEAD(&dev->ep0.urb_list); dev->ep0.desc.bLength = USB_DT_ENDPOINT_SIZE; diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 2fdafbcbe44c..18c743ce5ac5 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -2550,8 +2550,15 @@ static int dwc3_runtime_idle(struct device *dev) static int dwc3_suspend(struct device *dev) { struct dwc3 *dwc = dev_get_drvdata(dev); + struct platform_device *xhci = dwc->xhci; int ret; + if (xhci && xhci_sideband_check(xhci->dev.driver_data)) { + dev_dbg(dev, "device accessed via sideband\n"); + return 0; + } + + ret = dwc3_suspend_common(dwc, PMSG_SUSPEND); if (ret) return ret; @@ -2564,8 +2571,14 @@ static int dwc3_suspend(struct device *dev) static int dwc3_resume(struct device *dev) { struct dwc3 *dwc = dev_get_drvdata(dev); + struct platform_device *xhci = dwc->xhci; int ret; + if (xhci && xhci_sideband_check(xhci->dev.driver_data)) { + dev_dbg(dev, "device accessed via sideband\n"); + return 0; + } + pinctrl_pm_select_default_state(dev); pm_runtime_disable(dev); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 80047d0df179..e06d597ee3b0 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -26,6 +26,7 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> +#include <linux/usb/hcd.h> #include <linux/usb/role.h> #include <linux/ulpi/interface.h> @@ -1704,4 +1705,11 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc) { } #endif +#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND) +extern bool xhci_sideband_check(struct usb_hcd *hcd); +#else +static inline bool xhci_sideband_check(struct usb_hcd *hcd) +{ return false; } +#endif + #endif /* __DRIVERS_USB_DWC3_CORE_H */ diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 6e49ef1908eb..5fdbdf0c7f1a 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -456,6 +456,11 @@ static int xhci_plat_suspend_common(struct device *dev, struct pm_message pmsg) struct xhci_hcd *xhci = hcd_to_xhci(hcd); int ret; + if (pmsg.event == PM_EVENT_SUSPEND && xhci_sideband_check(hcd)) { + dev_dbg(dev, "device accessed via sideband\n"); + return 0; + } + if (pm_runtime_suspended(dev)) pm_runtime_resume(dev); @@ -499,6 +504,11 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg) struct xhci_hcd *xhci = hcd_to_xhci(hcd); int ret; + if (pmsg.event == PM_EVENT_RESUME && xhci_sideband_check(hcd)) { + dev_dbg(dev, "device accessed via sideband\n"); + return 0; + } + if (!device_may_wakeup(dev) && (xhci->quirks & XHCI_SUSPEND_RESUME_CLKS)) { ret = clk_prepare_enable(xhci->clk); if (ret) diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h index 6475130eac4b..432a040c81e5 100644 --- a/drivers/usb/host/xhci-plat.h +++ b/drivers/usb/host/xhci-plat.h @@ -30,4 +30,11 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, void xhci_plat_remove(struct platform_device *dev); extern const struct dev_pm_ops xhci_plat_pm_ops; +#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND) +extern bool xhci_sideband_check(struct usb_hcd *hcd); +#else +static inline bool xhci_sideband_check(struct usb_hcd *hcd) +{ return false; } +#endif + #endif /* _XHCI_PLAT_H */ diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c index 2dc651cd3d05..c82b5dbef2d7 100644 --- a/sound/usb/qcom/qc_audio_offload.c +++ b/sound/usb/qcom/qc_audio_offload.c @@ -1516,8 +1516,11 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle, mutex_lock(&chip->mutex); subs->opened = 0; mutex_unlock(&chip->mutex); + } else { + xhci_sideband_get(uadev[pcm_card_num].sb); } } else { + xhci_sideband_put(uadev[pcm_card_num].sb); info = &uadev[pcm_card_num].info[info_idx]; if (info->data_ep_pipe) { ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
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(+)