Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751975AbbEEBF5 (ORCPT ); Mon, 4 May 2015 21:05:57 -0400 Received: from mga14.intel.com ([192.55.52.115]:3853 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751170AbbEEBFt (ORCPT ); Mon, 4 May 2015 21:05:49 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,368,1427785200"; d="scan'208";a="705250304" Message-ID: <5548176C.7040502@linux.intel.com> Date: Tue, 05 May 2015 09:05:48 +0800 From: "Lu, Baolu" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Alan Stern CC: Greg Kroah-Hartman , Mathias Nyman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver References: In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5098 Lines: 136 Hi Alan, Thanks for your review comments. Below has my response inline. On 05/04/2015 10:28 PM, Alan Stern wrote: > On Mon, 4 May 2015, Lu Baolu wrote: > >> This patch adds a new entry pointer in hc_driver. With this new entry, >> USB core can notify host driver when something happens and host driver >> is willing to be notified. One use case of this interface comes from >> xHCI compatible host controller driver. > "Notify" is too generic. It's better to make the callback routine > specific to the activity in question. So this patch series should add > "device_suspend" and "device_resume" callbacks, not a general "notify" > callback. Fair enough. I will make this change in v2 if there is no objection. > >> 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 notify interface to notify xHCI driver whenever a USB >> device's power state is changed. >> >> 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 >> --- >> drivers/usb/core/generic.c | 10 ++++++++-- >> drivers/usb/core/hcd.c | 23 +++++++++++++++++++++++ >> include/linux/usb/hcd.h | 11 ++++++++++- >> 3 files changed, 41 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c >> index 358ca8d..92bee33 100644 >> --- a/drivers/usb/core/generic.c >> +++ b/drivers/usb/core/generic.c >> @@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg) >> /* Non-root devices don't need to do anything for FREEZE or PRETHAW */ >> else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW) >> rc = 0; >> - else >> + else { >> + hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg); >> rc = usb_port_suspend(udev, msg); >> + if (rc) >> + hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg); >> + } >> >> return rc; >> } >> @@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg) >> */ >> if (!udev->parent) >> rc = hcd_bus_resume(udev, msg); >> - else >> + else { >> rc = usb_port_resume(udev, msg); >> + hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg); >> + } > Don't you want to tell the HCD about the resume _before_ it happens? > After all, the devices endpoint will be used as soon as the resume > takes place. The order that software should do during device suspend/resume is defined in 4.15.1.1 of xHCI spec 1.1. Spec 4.15.1.1: Software shall stop all endpoints of a device using the Stop Endpoint Command and setting the Suspend (SP) flag to ?1? prior to selectively suspending a device. After the device is resumed software shall ring an endpoint?s doorbell to restart it. --end-- So the order looks like: tell hcd device suspend usb_port_suspend() usb_port_resume() tell hcd device resume > > And besides, this should be symmetrical with the code above, where you > tell the HCD about a suspend _after_ it happens. This was done in above change in generic_suspend(): @@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg) /* Non-root devices don't need to do anything for FREEZE or PRETHAW */ else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW) rc = 0; - else + else { + hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg); rc = usb_port_suspend(udev, msg); + if (rc) + hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg); + } return rc; } If usb_port_suspend() returns error, we should roll back HCD to the previous state. > > Alan Stern Thanks again, Baolu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/