Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/06/2015 10:35 PM, Alan Stern wrote:
> 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 <baolu.lu@xxxxxxxxxxxxxxx>
>
> > --- 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?

Yes, I agree. I will move this and below comments to xhci driver.

> > +	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?

This msg argument is valuable. XHCI spec defines a capability named FSC
(Force Save context Capability). When this capability is implemented, the
Save State operation (do during host suspend) shall save any cached Slot,
Endpoint, Stream or other Context information to memory. xHCI hcd could
use this "msg" to determine whether it needs to issue stop endpoint with
SP (suspend) bit set.

FSC is optional prior 1.1 and mandatory since 1.1. I have patches for 1.1
features in Mathias' gate. We could bring them up after we test them on
the real hardware.

> 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/

--
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/