From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753228AbbEFOfK (ORCPT ); Wed, 6 May 2015 10:35:10 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:33777 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751399AbbEFOfH (ORCPT ); Wed, 6 May 2015 10:35:07 -0400 Date: Wed, 6 May 2015 10:35:06 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Lu Baolu cc: Greg Kroah-Hartman , Mathias Nyman , , Subject: Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume In-Reply-To: <1430898002-24748-2-git-send-email-baolu.lu@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 6 May 2015, Lu Baolu wrote: > This patch adds two new entries in hc_driver. With these new entries, > USB core can notify host driver when a USB device is about to suspend > or just resumed. > > The xHCI spec is designed to allow an xHC implementation to cache the > endpoint state. Caching endpoint state allows an xHC to reduce latency > when handling ERDYs and other USB asynchronous events. However holding > this state in xHC consumes resources and power. The xHCI spec designs > some methods through which host controller driver can hint xHC about > how to optimize its operation, e.g. to determine when it holds state > internally or pushes it out to memory, when to power down logic, etc. > > When a USB device is going to suspend, states of all endpoints cached > in the xHC should be pushed out to memory to save power and resources. > Vice versa, when a USB device resumes, those states should be brought > back to the cache. USB core suspends or resumes a USB device by sending > set/clear port feature requests to the parent hub where the USB device > is connected. Unfortunately, these operations are transparent to xHCI > driver unless the USB device is plugged in a root port. This patch > utilizes the callback entries to notify xHCI driver whenever a USB > device suspends or resumes. > > It is harmless if a USB devices under USB 3.0 host controller suspends > or resumes without a notification to hcd driver. However there may be > less opportunities for power savings and there may be increased latency > for restarting an endpoint. The precise impact will be different for > each xHC implementation. It all depends on what an implementation does > with the hints. > > Signed-off-by: Lu Baolu > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -3144,6 +3144,14 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > goto err_lpm3; > } > > + /* > + * Call back to hcd if it expects. xHCI compatible host controller > + * driver expects to be notified prior to selectively suspending a > + * device. xHCI hcd could optimize the endpoint cache for power > + * saving purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information. > + */ Doesn't this comment belong in the xhci-hcd source code rather than the hub driver source code? > + hcd_suspend_notify(udev, msg); > + > /* see 7.1.7.6 */ > if (hub_is_superspeed(hub->hdev)) > status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3); > @@ -3169,6 +3177,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > if (status) { > dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status); > > + hcd_resume_notify(udev, msg); > + > /* Try to enable USB3 LPM and LTM again */ > usb_unlocked_enable_lpm(udev); > err_lpm3: > @@ -3422,6 +3432,12 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) > } > > SuspendCleared: > + /* Call back to hcd if it expects. xHCI compatible host controller > + * driver expects to be notified after a device is resumed. xHCI > + * hcd could optimize the endpoint cache for latency reducing > + * purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information. > + */ Same for this comment. > + hcd_resume_notify(udev, msg); > if (status == 0) { > udev->port_is_suspended = 0; > if (hub_is_superspeed(hub->hdev)) { > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h > index 68b1e83..621d9b7 100644 > --- a/include/linux/usb/hcd.h > +++ b/include/linux/usb/hcd.h > @@ -383,7 +383,13 @@ struct hc_driver { > int (*find_raw_port_number)(struct usb_hcd *, int); > /* Call for power on/off the port if necessary */ > int (*port_power)(struct usb_hcd *hcd, int portnum, bool enable); > - > + /* Call back to hcd when a USB device is going to suspend or just > + * resumed. > + */ > + void (*device_suspend)(struct usb_hcd *, struct usb_device *udev, > + pm_message_t msg); > + void (*device_resume)(struct usb_hcd *, struct usb_device *udev, > + pm_message_t msg); > }; Your callbacks don't use the msg argument. What makes you think it is needed? Alan Stern