Message-ID: <Pine.LNX.4.44L0.1505111006080.1411-100000@iolanthe.rowland.org>
Date:	Mon, 11 May 2015 10:25:12 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Lu, Baolu" <baolu.lu@...ux.intel.com>
cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Mathias Nyman <mathias.nyman@...el.com>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

On Sat, 9 May 2015, Lu, Baolu wrote:

> >> If FSC is supported,  the cached Slot, Endpoint, Stream, or other
> >> Context information are also saved.
> >>
> >> Hence, when FSC is supported, software does not have to issue Stop
> >> Endpoint Command to push public and private endpoint state into
> >> memory as part of system suspend process.
> > Why do you have to push this state into memory at all?  Does the
> > controller hardware lose the cached state information when it is in low
> > power?
> 
> I don't think controller hardware will lose the cached state information
> when it is in low power. But since cache in controller consumes power
> and resources, by pushing state into memory, hardware can power
> off the cache logic during suspend. Hence more power saving gains.
> 
> >
> >> The logic in xhci_device_suspend() will look like:
> >>
> >> if xhci_device_suspend() callback was called due to system suspend,
> >> (mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
> >> the xHC implementation, xhci_device_suspend() could ignore stop
> >> endpoint command, since CSS will be done in xhc_suspend() later and
> >> where all the endpoint caches will be pushed to memory.
> > I still don't understand this.  You said earlier that according
> > to section 4.15.1.1 of the xHCI spec, the endpoint rings should
> > _always_ be stopped with SP set when a device is suspended.  Now you're
> 
> The intention of stop endpoint with SP set is to tell hardware that
> "a device is going to suspend, hardware don't need to contain the
> endpoint state in internal cache anymore". Hardware _could_ use
> this hint to push endpoint state into memory to reduce power
> consumption.
> 
> > saying that they don't need to be stopped during a system suspend if
> > the controller supports FSC.  (Or maybe you're saying they need to be
> > stopped but SP doesn't need to be set -- it's hard to tell.)
> 
> Even FSC is supported, controller hardware still need to push cached
> endpoint state into memory when a USB device is suspended. The
> difference is when FSC is enforced, CSS command will push any
> cached endpoint state into memory unconditionally.

You said above that the hardware _could_ push endpoint state into
memory.  Now you're saying it _needs_ to do this!  Make up your mind.


> 
> So, when xhci_device_suspend() knows that CSS command will be
> executed later and CSS command will push cached endpoint state
> into memory (a.k.a. FSC is enforced), it could skip issuing stop
> endpoint command with SP bit set to avoid duplication and reduce
> the suspend time.
> 
> This is the case for system suspend since CSS command is part of
> xhci_suspend() and xhci_suspend() will be executed after all USB
> devices have been suspended. But it's not case for run-time suspend
> (auto-pm) since USB device suspend and host controller suspend
> are independent for run-time case.
> 
> That's the reason why I wanted to keep 'msg' parameter. But just as
> Greg said, we don't need to keep a parameter when it's not used
> and can add it later when it is required.
> 
> >
> > So which is it?  Do you need to stop the endpoint rings?  Is it okay
> > not to set SP?
> 
> "stop endpoint" and "stop endpoint with SP set" serve different purposes
> in Linux xhci driver as my understanding. "stop endpoint" command is
> used to stop a active ring when upper layer want to cancel a URB.
> "stop endpoint with SP set" is used to hint hardware that a USB is going
> to suspend. Hence "stop endpoint with SP set" must be executed in case
> that the transfer ring is empty.

(How does the contents of the transfer ring affect anything?  Besides, 
there are never any active URBs when a device gets suspended, so the 
transfer ring will _always_ be empty at such times.)

This is still extremely confusing.  You're not doing a good job of 
explaining the situation clearly and logically.

Let's see if I understand it correctly:

	When the controller goes into suspend, you want the cache to
	be pushed into memory so that the cache can be powered down,
	thereby saving additional energy.

	If the hardware supports FSC, this will happen automatically.

	If the hardware doesn't support FSC, the cached data won't get
	pushed to memory unless the driver tells the controller to do 
	so at the time the device is suspended.  But this will slow 
	things down, so the driver should avoid doing it when it's not 
	needed.

	During system suspend you know in advance that the controller
	will be suspended.  Therefore the driver should push the cache 
	to memory if the hardware doesn't support FSC.  During runtime 
	suspend you don't know in advance whether the controller will 
	be suspended, so the driver should not push the cache to 
	memory.

	But what happens in the case where all the devices have gone 
	into runtime suspend, so the controller also goes into runtime
	suspend, and the hardware doesn't support FSC?  It seems that
	in this case the cache would have to remain powered on.

Also, it's still not clear what the driver needs to do differently in
order to push out the cached data.  You have managed to imply both:

	Issuing Stop Endpoint with SP set is mandatory whenever
	a device is suspended, and

	The SP bit is what tells the controller to push the endpoint
	state to memory.

This doesn't make sense.

One last question: How much extra time does it take to push the cached 
data to memory?  Bear in mind that suspending a device is not a rapid 
operation; if pushing the data takes no more than a few microseconds, I 
think it would be worthwhile to do it always, even when it's not 
necessary.

Alan Stern

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

Powered by blists - more mailing lists