Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Alan Stern]

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.

> 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 <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>  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.

And besides, this should be symmetrical with the code above, where you
tell the HCD about a suspend _after_ it happens.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/