Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Sarah Sharp]

Hi Alexis,

This is a quirk for your TI host controller because it doesn't properly
give a port status event for the link status change to compliance mode,
correct?

First, you need to add that background to your patch description, and
describe what triggers this behavior and how frequently it can occur.

Second, you need to make a separate xHCI quirk for your host controller,
set it based on the PCI vendor and device ID in xhci-pci.c, and only arm
the timer if the quirk is set.  We don't need this timer for any other
host controllers, so it should run only for your host.

If you need an example, look through the xHCI driver for
XHCI_EP_LIMIT_QUIRK.

On Tue, Jun 19, 2012 at 05:12:39PM -0500, Alexis Cortes wrote:
> This change creates a timer that monitors the PORTSC registers and recovers
> the
> port by issuing a Warm reset everytime Compliance mode is detected.

Your patch is line wrapped and can't be applied.  Please resend with a
mail client that won't mangle your patches.  I personally use mutt, but
you can take a look at Documentation/email-clients.txt for other
suggestions.

> Signed-off-by: Alexis R. Cortes <alexis.cortes@ti.com>
> ---
>  drivers/usb/host/xhci.c |   39 +++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci.h |    4 ++++
>  2 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index afdc73e..a43e52b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -397,6 +397,39 @@ static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
>  
>  #endif
>  
> +static void compliance_mode_rcvry(unsigned long arg)
> +{
> +	struct xhci_hcd *xhci;
> +	u32 temp;
> +	int i;
> +
> +	xhci = (struct xhci_hcd *) arg;
> +	for (i = 0; i < xhci->num_usb3_ports; i++) {
> +		temp = xhci_readl(xhci, xhci->usb3_ports[i]);
> +		if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {

Um, what happens if the timer fires while the xHCI host is in PCI D3
because it has been auto-suspended?  All the registers read as
0xfffffff.  You should make sure to stop and restart the timer when the
the host is suspended.

> +			temp = xhci_port_state_to_neutral(temp);
> +			temp |= PORT_WR;
> +			xhci_writel(xhci, temp, xhci->usb3_ports[i]);
> +			xhci_dbg(xhci, "Compliance Mode Detected. Warm "
> +				       "reset performed to port %d for "
> +				       "recovery.\n", i + 1);

Instead of doing the warm reset here, you need to let the USB core
handle it. Here, you should kick khubd for the USB 3.0 roothub by
calling usb_hcd_poll_rh_status().  Look at
xhci-ring.c:handle_port_status() for an example.

Then in the xhci-hub.c code that checks for port changes, you should
fake a link status change.  The USB core will notice the status change
and see the port's link status of compliance.  Then the USB core will
take care of issuing the warm reset and doing a proper job of timing it.

You can see this sort of process by looking at the recent CAS patch:
http://marc.info/?l=linux-usb&m=134002582807373&w=2

Your patch will need to be built on top of that one, since that patch
adds support for issuing a warm reset when compliance mode is detected.

> +		}
> +	}
> +	mod_timer(&xhci->comp_mode_rcvry_timer,
> +		  jiffies + msecs_to_jiffies(COMP_MODE_RCVRY_TIMEOUT *
> 1000));

Everywhere you use COMP_MODE_RCVRY_TIMEOUT you multiply it by 1000.  Why
not just rename it to COMP_MODE_RCVRY_MSECS and define it to 2000?

> +}
> +
> +static void compliance_mode_rcvry_timer_init(struct xhci_hcd *xhci)
> +{
> +	init_timer(&xhci->comp_mode_rcvry_timer);
> +	xhci->comp_mode_rcvry_timer.data = (unsigned long) xhci;
> +	xhci->comp_mode_rcvry_timer.function = compliance_mode_rcvry;
> +	xhci->comp_mode_rcvry_timer.expires = jiffies +
> +		msecs_to_jiffies(COMP_MODE_RCVRY_TIMEOUT * 1000);
> +	add_timer(&xhci->comp_mode_rcvry_timer);
> +	xhci_dbg(xhci, "Compliance Mode Recovery Timer Initialized.\n");
> +}
> +

This function should immediately return if your new quirk isn't set.

>  /*
>   * Initialize memory for HCD and xHC (one-time init).
>   *
> @@ -420,6 +453,9 @@ int xhci_init(struct usb_hcd *hcd)
>  	retval = xhci_mem_init(xhci, GFP_KERNEL);
>  	xhci_dbg(xhci, "Finished xhci_init\n");
>  
> +	/* Initializing Compliance Mode Recovery Timer */
> +	compliance_mode_rcvry_timer_init(xhci);
> +
>  	return retval;
>  }
>  
> @@ -628,6 +664,9 @@ void xhci_stop(struct usb_hcd *hcd)
>  	del_timer_sync(&xhci->event_ring_timer);
>  #endif
>  
> +	/* Deleting Compliance Mode Recovery Timer */
> +	del_timer_sync(&xhci->comp_mode_rcvry_timer);
> +

You need to put this into a new function that also returns immediately
if your quirk isn't set.

>  	if (xhci->quirks & XHCI_AMD_PLL_FIX)
>  		usb_amd_dev_put();
>  
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index de3d6e3..0f11cb8 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1506,6 +1506,10 @@ struct xhci_hcd {
>  	unsigned		sw_lpm_support:1;
>  	/* support xHCI 1.0 spec USB2 hardware LPM */
>  	unsigned		hw_lpm_support:1;
> +	/* Compliance Mode Recovery Timer */
> +	struct timer_list comp_mode_rcvry_timer;
> +/* Compliance Mode Timer Triggered every 2 seconds */
> +#define COMP_MODE_RCVRY_TIMEOUT 2

How often do you really need this timer to run?  Do you really want to
wake your CPU out of deep C states every two seconds?  Is there any way
you can narrow the scope of when the timer runs so that it doesn't run
that often, or increase this timeout to something like 10 seconds?

Sarah Sharp

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Sarah Sharp]

On Wed, Jun 20, 2012 at 05:24:28PM -0500, Alexis Cortes wrote:
> Hi Sarah,
> 
> First of all, thanks for your reply.
> 
> On the subject, although the workaround is implemented on the host
> controller, the problem is not with our host controller but with our
> SN65LVPE502CP redriver (http://www.ti.com/product/sn65lvpe502cp).

Is a redriver something motherboard manufacturers choose?  Or is a
redriver always sold paired with a particular host controller chip?

> As
> mentioned in our previous email, we discovered a timing issue in that
> version of our USB3.0 re-driver that can delay the negotiation between a
> device and the host past the usual handshake timeout, and if that happens on
> the first insertion, the host controller port will enter in Compliance mode
> as per the xHCI spec (Section 4.19.4 of the xHCI 1.0 spec "If a timeout is
> detected on the first LFPS handshake, the port transitions to the Compliance
> state and NO change flag is set");

How often is this timing issue hit?  On every plug in?  Every 100 plug
ins?  Is it specific to which devices are plugged in or can it be
triggered for any device?  Does the redriver get stuck in this state, or
is it transient?

Basically, will the user say, "Sometimes I plug the USB device in, and
it doesn't work, but then I unplug and replug it, and it works"?

> in conclusion No host controller will
> generate a Port Status Change (PSC) event (regardless of the host vendor) as
> a result of a transition to compliance mode and any port that has this piece
> of hardware between the root port and the physical port, it is most likely
> to suffer of this condition. 

That seems like a spec bug.  SW really needs to know when a failed link
training happens.  I've emailed the xHCI spec architect, and we'll see
if we can get an errata to set the PLC bit when the port goes into
compliance mode.

> Unfortunately there is not a way to programmatically detect if the re-driver
> is present on the system, and since it might affect any host controller, I'm
> afraid this workaround can't be limited on the driver to specific hardware. 

Ok, then make it a module parameter that is off by default.  Users who
find they have this issue can reload the driver with the timer on, and
add the module parameter to their grub linux boot line.  If we find that
the redriver is used always for one particular host vendor/revision,
we'll add a quirk for it.

But I really don't want an extra timer running and killing power
management for hosts that don't need this work around.

You should look at using slacktimers so that the timer can be grouped
with other timers.  It's not imperative that your timer runs exactly
every two seconds, so that might save power management a bit.  You
probably set the timer slack up to 500ms or a second without much user
experience degradation.  Here's an LWN article on slacktimers:

http://lwn.net/Articles/369549/

> Regarding the patch description I'm planning to add this: "This patch is
> intended to work around a known issue on the SN65LVPE502CP USB3.0 re-driver
> that can delay the negotiation between a device and the host past the usual
> handshake timeout, and if that happens on the first insertion, the host
> controller port will enter in Compliance mode as per the xHCI spec. The
> patch creates a timer which polls every 2 seconds the link state of each
> host controller's port (this by reading the PORTSC register) and recovers
> the port by issuing a Warm reset every time Compliance mode is detected."

Sounds fine.

> >> +			temp = xhci_port_state_to_neutral(temp);
> >> +			temp |= PORT_WR;
> >> +			xhci_writel(xhci, temp, xhci->usb3_ports[i]);
> >> +			xhci_dbg(xhci, "Compliance Mode Detected. Warm "
> >> +				       "reset performed to port %d for "
> >> +				       "recovery.\n", i + 1);
> > 
> > Instead of doing the warm reset here, you need to let the USB core handle
> it.
> > Here, you should kick khubd for the USB 3.0 roothub by calling
> > usb_hcd_poll_rh_status().  Look at
> > xhci-ring.c:handle_port_status() for an example.
> > 
> > Then in the xhci-hub.c code that checks for port changes, you should fake
> a
> > link status change.  The USB core will notice the status change and see
> the
> > port's link status of compliance.  Then the USB core will take care of
> issuing
> > the warm reset and doing a proper job of timing it.
> > 
> > You can see this sort of process by looking at the recent CAS patch:
> > http://marc.info/?l=linux-usb&m=134002582807373&w=2
> > 
> > Your patch will need to be built on top of that one, since that patch adds
> > support for issuing a warm reset when compliance mode is detected.
> 
> I made the patch the easiest way I could think off. I will explore this path
> you're proposing and change my patch. Has this patch you're mentioning
> already been committed to the usb-next branch?

It hasn't been queued to Greg yet.  I was waiting to see if it needed
another revision, but it looks like Andiry acked it.  I'm going to be
sending it off to Greg for the usb-linus branch, not the usb-next
branch, because it's a bug fix, not a new feature.  It's marked for
stable as well.

Your patch would come through my tree, so you should just base your
patch on my for-usb-linus branch:

http://git.kernel.org/?p=linux/kernel/git/sarah/xhci.git;a=shortlog;h=refs/heads/for-usb-linus

> >> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index
> >> de3d6e3..0f11cb8 100644
> >> --- a/drivers/usb/host/xhci.h
> >> +++ b/drivers/usb/host/xhci.h
> >> @@ -1506,6 +1506,10 @@ struct xhci_hcd {
> >>  	unsigned		sw_lpm_support:1;
> >>  	/* support xHCI 1.0 spec USB2 hardware LPM */
> >>  	unsigned		hw_lpm_support:1;
> >> +	/* Compliance Mode Recovery Timer */
> >> +	struct timer_list comp_mode_rcvry_timer;
> >> +/* Compliance Mode Timer Triggered every 2 seconds */ #define
> >> +COMP_MODE_RCVRY_TIMEOUT 2
> > 
> > How often do you really need this timer to run?  Do you really want to
> wake
> > your CPU out of deep C states every two seconds?  Is there any way you can
> > narrow the scope of when the timer runs so that it doesn't run that often,
> or
> > increase this timeout to something like 10 seconds?
> 
> If this compliance mode gets to happen, 10 seconds could be too much waiting
> for an end user to see proper enumeration of their device, I think.

Meh, the USB storage driver waits five seconds for all devices, and that
seems tolerable to me.  But I'm fine with it being two seconds if it's a
module parameter that defaults to false.

> Also I'm
> planning to disable the timer once all of host's ports have enter U0 (since
> a port can't enter compliance mode once it has previously entered U0) to
> reduce timer's overload.

So it will still run if there is any empty USB port on the system?  And
what if some of the ports are in other link states, like U3?

Sarah Sharp

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Greg KH]

On Wed, Jun 20, 2012 at 05:07:34PM -0700, Sarah Sharp wrote:
> > Unfortunately there is not a way to programmatically detect if the re-driver
> > is present on the system, and since it might affect any host controller, I'm
> > afraid this workaround can't be limited on the driver to specific hardware. 
> 
> Ok, then make it a module parameter that is off by default.  Users who
> find they have this issue can reload the driver with the timer on, and
> add the module parameter to their grub linux boot line.  If we find that
> the redriver is used always for one particular host vendor/revision,
> we'll add a quirk for it.
> 
> But I really don't want an extra timer running and killing power
> management for hosts that don't need this work around.

I don't want that either, but I really don't want a new module parameter
that no one is going to know that they need to set.

I think the real solution is finding what pci devices this is a problem
with, and using a quirk that way.

thanks,

greg k-h

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Sarah Sharp]

On Thu, Jun 21, 2012 at 12:31:12PM -0500, Alexis Cortes wrote:
> Hi Greg,
> 
> I understand your concerns, however as I mentioned before, any xHCI host
> that has this particular re-driver between its root-ports and the physical
> ports of the system will be subject to suffer of this compliance mode issue
> (once the port has entered compliance mode, it becomes unusable so no device
> that is plugged to that port will work until a warm reset is applied to it).
> For a system that has this re-driver, this problem could hit about 20%-40%
> of the times (however this percentage is subject to the quality of the
> internal connection) and unfortunately there is no way to programmatically
> detect if this re-driver is on the system.
> 
> As Sarah proposed, we certainly can apply this patch as a module parameter
> disabled by default and let know our clients that we know are using this
> re-driver to enable the feature to avoid the issue.

I don't think that would work very well.  Are those clients supposed to
notify Linux OSVs when a system will ship with that redriver so they can
turn it on for Linux preloads of those systems?  What about the average
Linux user who installs Linux themselves?

An alternative approach, since you know which clients are using the
re-driver, is to just add a quirk, and get them to tell us when they're
shipping a system with your redriver.  Then we can turn it on in the
mainline kernel, all Linux distros will pick it up, and we will avoid
disgruntled users.

Or we can just modify the timer to a more reasonable value like 10
seconds, and users will just have to put up with the longer enumeration
times.

Greg, what about exporting a sysfs file to change the polling interval?
We could run the timer every 2 seconds by default, but get powertop to
add a new setting for turning the interval off.

Sarah Sharp

> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Wednesday, June 20, 2012 7:33 PM
> > To: Sarah Sharp
> > Cc: Alexis Cortes; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org;
> > 'Quach, Brian'; 'Llamas, Jorge'
> > Subject: Re: [PATCH] usb: host: xhci: Compliance Mode port recovery
> > 
> > On Wed, Jun 20, 2012 at 05:07:34PM -0700, Sarah Sharp wrote:
> >> > Unfortunately there is not a way to programmatically detect if the
> >> > re-driver is present on the system, and since it might affect any
> >> > host controller, I'm afraid this workaround can't be limited on the
> > driver to specific hardware.
> >>
> >> Ok, then make it a module parameter that is off by default.  Users who
> >> find they have this issue can reload the driver with the timer on, and
> >> add the module parameter to their grub linux boot line.  If we find
> >> that the redriver is used always for one particular host
> >> vendor/revision, we'll add a quirk for it.
> >>
> >> But I really don't want an extra timer running and killing power
> >> management for hosts that don't need this work around.
> > 
> > I don't want that either, but I really don't want a new module parameter
> > that no one is going to know that they need to set.
> > 
> > I think the real solution is finding what pci devices this is a problem
> > with, and using a quirk that way.
> > 
> > thanks,
> > 
> > greg k-h
> > 
> >
> 

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

['Greg KH']

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Jun 21, 2012 at 12:31:12PM -0500, Alexis Cortes wrote:
> As Sarah proposed, we certainly can apply this patch as a module parameter
> disabled by default and let know our clients that we know are using this
> re-driver to enable the feature to avoid the issue.

Who is a "client"?  And who is going to modify the installer of their
distro in order to automatically enable this option?

That's the problem with options, you never know if you need to turn it
on or not, so I _really_ don't ever want to add any more, the kernel
should "just know" if it needs to be enabled or not.  Surely there is
some way for the kernel to determine if this is your code/hardware
running on the platform or not, right?  No signature in the system
anywhere?  PCI id? DMI table?  ACPI table?  BIOS signature?  Something
else?

You really don't want to be responsible for dealing with 10+ distros in
telling them when they should, or should not, enable this option.  So
please don't create it in the first place.

greg k-h

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

['Greg KH']

On Thu, Jun 21, 2012 at 05:08:58PM -0700, Sarah Sharp wrote:
> On Thu, Jun 21, 2012 at 12:31:12PM -0500, Alexis Cortes wrote:
> > Hi Greg,
> > 
> > I understand your concerns, however as I mentioned before, any xHCI host
> > that has this particular re-driver between its root-ports and the physical
> > ports of the system will be subject to suffer of this compliance mode issue
> > (once the port has entered compliance mode, it becomes unusable so no device
> > that is plugged to that port will work until a warm reset is applied to it).
> > For a system that has this re-driver, this problem could hit about 20%-40%
> > of the times (however this percentage is subject to the quality of the
> > internal connection) and unfortunately there is no way to programmatically
> > detect if this re-driver is on the system.
> > 
> > As Sarah proposed, we certainly can apply this patch as a module parameter
> > disabled by default and let know our clients that we know are using this
> > re-driver to enable the feature to avoid the issue.
> 
> I don't think that would work very well.  Are those clients supposed to
> notify Linux OSVs when a system will ship with that redriver so they can
> turn it on for Linux preloads of those systems?  What about the average
> Linux user who installs Linux themselves?
> 
> An alternative approach, since you know which clients are using the
> re-driver, is to just add a quirk, and get them to tell us when they're
> shipping a system with your redriver.  Then we can turn it on in the
> mainline kernel, all Linux distros will pick it up, and we will avoid
> disgruntled users.
> 
> Or we can just modify the timer to a more reasonable value like 10
> seconds, and users will just have to put up with the longer enumeration
> times.
> 
> Greg, what about exporting a sysfs file to change the polling interval?
> We could run the timer every 2 seconds by default, but get powertop to
> add a new setting for turning the interval off.

Ick, a sysfs file is almost as bad as a kernel module option, how are
you going to tell users / distros when to turn it off or not if you
don't know if it is needed or not?

We really need a way to determine the hardware here.

Alexis, what are you doing on Windows for this?  Surely you can't be
turning a timer on every 2 seconds for all Windows systems in the world,
are you?

thanks,

greg k-h

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Sarah Sharp]

On Thu, Jun 21, 2012 at 01:52:20PM -0500, Alexis Cortes wrote:
> Hi Sarah,
> > How often is this timing issue hit?  On every plug in?  Every 100 plug
> ins?
> > Is it specific to which devices are plugged in or can it be triggered for
> > any device?  Does the redriver get stuck in this state, or is it
> transient?
> 
> It depends on the manufacturing quality of the port terminations and
> internal connections; from our testing we determined the issue hits about
> 20% to 40% of the times. This issue will happen only at first insertion,
> once a port has entered U0, it will never enter compliance (until the system
> is rebooted or sent to S4, i.e. when the redriver is power-cycled).
> 
> This issue can be triggered by any SS device, and once port enters
> compliance, no other device of any kind/speed will work on this port and the
> port will remain unusable until a warm reset is issued or system is power
> cycled.

If I'm understanding you correctly, when this issue is hit once, and we
warm reset the port, the port will never go into compliance mode again.
So even if the user unplugs the device, and the port goes back into
polling, the next plug in will never go into compliance mode, until
after an S4 resume or a power-cycle.  Is that correct?

If so, I see no reason why we shouldn't increase the timer to 10
seconds.  The user will see one very slow enumeration per port, and then
all the other enumerations will work at the normal speed.

> Let me explain this more clearly:
> There's no way to know which ports of the host controller have a redriver
> between the root-port and the physical port, so we need to poll all the
> ports.
> If a port that has the redriver has previously enter to U0 at least once (it
> doesn't matter the actual state of the port, only if it has previously
> entered U0), then the compliance mode issue won't happen (until the system
> is power-cycled or if it resumes from S4). So if all ports have entered at
> least once to U0, I can disable the timer because I know for sure the
> compliance issue won't happen at any port.

Ok, this statement makes it more clear.  Based on that, I think a 10
second timer would work.

I think you'll need a dynamically allocated port bitmask array in the
xhci_hcd to keep track of which ports have entered U0 at least once
since the module was loaded, or we came back from S4.  Then you can
disable the timer if all the ports have come into U0.

This is really going to suck from a power management perspective.  "How
do I stop the timer from polling?"  "Oh, you plug a device into every
single port"  "But there's an internal port with nothing connected!"
"Uhhh..."

Greg, are you sure wouldn't consider a sysfs file for changing the
polling interval?

Sarah Sharp

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Sarah Sharp]

On Thu, Jun 21, 2012 at 06:48:25PM -0700, 'Greg KH' wrote:
> On Thu, Jun 21, 2012 at 05:08:58PM -0700, Sarah Sharp wrote:
> > Greg, what about exporting a sysfs file to change the polling interval?
> > We could run the timer every 2 seconds by default, but get powertop to
> > add a new setting for turning the interval off.
> 
> Ick, a sysfs file is almost as bad as a kernel module option, how are
> you going to tell users / distros when to turn it off or not if you
> don't know if it is needed or not?

The same way users discover whether their USB devices break if
auto-suspend is turned on.  They go into powertop, and change the line
for their USB device from "BAD" to "GOOD".  Then they notice their mouse
suddenly stops responding to movement, and they change the powertop
settings back.

In the same way, there would be a line like "Stop xHCI port polling
timer" that they would toggle.  Later, if devices under the ports
stopped enumerating, they would change it back to "BAD" and just put up
with the polling.

I agree it's not a very good system, and adding a quirk to the xHCI
driver is a better solution.  Ideally, TI would just fix their redriver.

> We really need a way to determine the hardware here.
> 
> Alexis, what are you doing on Windows for this?  Surely you can't be
> turning a timer on every 2 seconds for all Windows systems in the world,
> are you?

Yes, what are you going to do for Windows 8 systems that have official
Microsoft USB 3.0 support?  Make your customers ship a driver with the
polling turned on?

Sarah Sharp

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

[Greg KH]

On Fri, Jun 22, 2012 at 09:32:14AM -0700, Sarah Sharp wrote:
> This is really going to suck from a power management perspective.  "How
> do I stop the timer from polling?"  "Oh, you plug a device into every
> single port"  "But there's an internal port with nothing connected!"
> "Uhhh..."
> 
> Greg, are you sure wouldn't consider a sysfs file for changing the
> polling interval?

What I really want is a list of the PCI devices that this is affected
in, I'm sure that this can be found, as what else would Windows be
doing?

Alexis, what are you going to do about this for Windows?  Why not solve
it the same way here for Linux?

thanks,

greg k-h

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

['Greg KH']

On Tue, Jun 26, 2012 at 12:26:29PM -0500, Alexis Cortes wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Friday, June 22, 2012 11:47 AM
> > To: Sarah Sharp
> > Cc: Alexis Cortes; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org;
> > 'Quach, Brian'; 'Llamas, Jorge'
> > Subject: Re: [PATCH] usb: host: xhci: Compliance Mode port recovery
> > 
> > On Fri, Jun 22, 2012 at 09:32:14AM -0700, Sarah Sharp wrote:
> >> This is really going to suck from a power management perspective.
> >> "How do I stop the timer from polling?"  "Oh, you plug a device into
> >> every single port"  "But there's an internal port with nothing
> connected!"
> >> "Uhhh..."
> >>
> >> Greg, are you sure wouldn't consider a sysfs file for changing the
> >> polling interval?
> > 
> > What I really want is a list of the PCI devices that this is affected in,
> > I'm sure that this can be found, as what else would Windows be doing?
> 
> The redriver is completely transparent to the OS and the issue will affect
> all xHCI host controllers.

Ouch.  There's no way to detect by known PCI ids of the devices that you
know have been shipped with this in it?

> > Alexis, what are you going to do about this for Windows?  Why not solve it
> > the same way here for Linux?
> 
> We have implemented a 2 second polling in our Windows XP/Vista/7 xHCI driver
> and are currently engaging with Microsoft to implement a workaround for the
> inbox Win8 driver.

Good luck with that :)

Seriously, 2 seconds isn't that bad, but it can be noticable on some
systems that are in idle mode, as I'm sure you have measured.

greg k-h

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

['Greg KH']

On Tue, Jul 10, 2012 at 05:32:35PM -0500, Alexis Cortes wrote:
> Hi Sarah & Greg,
> 
> I made another patch for this issue following your recommendations. The only
> thing that is left is the way the patch is going to be implemented on the
> kernel (module parameter, sysfs...), which is still in discussion. The
> changes I made for this patch are as follows:
> 
> * Changed #define COMP_MODE_RCVRY_TIMEOUT 2 by #define COMP_MODE_RCVRY_MSECS
> 2000.
> * Timer implemented as a Slack Timer.
> * Stop and Restart the timer when the host is suspended.
> * Let the USB core handle the warm reset.
> * Stop timer when all ports have entered U0.

That's a much nicer version, thanks.

Few minor corrections below:

> [PATCH] usb: host: xhci: Compliance Mode Port Recovery

Better subject: "Fix compliance mode on SN65LVPE502CP hardware"?

As this is a fix for broken hardware, right?

> +	} else {
> +		/* If CAS bit isn't set but the Port is already at
> +		 * Compliance Mode, fake a connection so the USB core
> +		 * notices the Compliance state and resets the port

Add "This resolves an issue with the..." describing the hardware
problem?

> +					xhci_dbg(xhci, "Compliance Mode
> Recovery Timer "
> +						       "Deleted. All USB3
> ports have "
> +						       "entered U0 at least
> once.\n");

Keep the string all on one line.

> +/*Compliance Mode Recovery Patch*/

Why is this comment needed?

> +static void compliance_mode_rcvry(unsigned long arg)

Vowels are free, please use them :)

> +{
> +	struct xhci_hcd *xhci;
> +	struct usb_hcd *hcd;
> +	u32 temp;
> +	int i;
> +
> +	xhci = (struct xhci_hcd *) arg;

No space needed before "arg".

> +
> +	for (i = 0; i < xhci->num_usb3_ports; i++) {
> +		temp = xhci_readl(xhci, xhci->usb3_ports[i]);
> +		if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {
> +			/* Compliance Mode Detected. Letting USB Core handle
> +			 * the Warm Reset */

Multi-line comments are usually in this form:
			/*
			 * Compliance Mode Detected. Letting USB Core handle
			 * the Warm Reset.
			 */

> +			xhci_dbg(xhci, "Compliance Mode Detected on port %d!
> "
> +					"Attempting recovery routine.\n", i

Don't spread strings across lines, it makes it harder to grep for them.

> +static void compliance_mode_rcvry_timer_init(struct xhci_hcd *xhci)
> +{
> +	xhci->port_status_u0 = 0;
> +	init_timer(&xhci->comp_mode_rcvry_timer);
> +	xhci->comp_mode_rcvry_timer.data = (unsigned long) xhci;
> +	xhci->comp_mode_rcvry_timer.function = compliance_mode_rcvry;
> +	xhci->comp_mode_rcvry_timer.expires = jiffies +
> +		msecs_to_jiffies(COMP_MODE_RCVRY_MSECS);
> +	set_timer_slack(&xhci->comp_mode_rcvry_timer, HZ);

That seems like a pretty strict slack time.  Can't you make it much
larger?  Like at least COMP_MODE_RCVRY_MSECS?  You don't need a precise
timer here at all, so give it as much room to be delayed as possible.

> +	add_timer(&xhci->comp_mode_rcvry_timer);
> +	xhci_dbg(xhci, "Compliance Mode Recovery Timer Initialized.\n");
> +}
> +
>  /*
>   * Initialize memory for HCD and xHC (one-time init).
>   *
> @@ -420,6 +464,9 @@ int xhci_init(struct usb_hcd *hcd)
>  	retval = xhci_mem_init(xhci, GFP_KERNEL);
>  	xhci_dbg(xhci, "Finished xhci_init\n");
>  
> +	/* Initializing Compliance Mode Recovery Data */
> +	compliance_mode_rcvry_timer_init(xhci);

There's really no way we can detect this based on the hardware on the
system?  Firmware version number?  PCI ids?  DMI strings?  BIOS
versions?  Hardware platform types?  Processor types?  Something?
Anything?

What did Microsoft say in your proposal to them to add this timer for
every Windows system using xhci?

thanks,

greg k-h

Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

['Greg KH']

On Mon, Jul 30, 2012 at 03:22:52PM -0500, Alexis Cortes wrote:
> Hi Greg,
> 
> I'm sorry for my late response on this. First of all thanks for your reply
> and your feedback :) 
> 
> We have been discussing with one of our major customers the possibility of
> identifying the platforms with the failing piece of hardware
> (SN65LVPE502CP), and as you suggested they have provided some DMI strings we
> can check in order to identify the platforms where those devices were
> installed. 
> 
> I have modified the patch so it will be executed only on those platforms
> reporting the specified DMI strings. I also applied some other suggestions
> you made on your previous email.
> 
> I would really appreciate if you could take a look at the patch and give me
> your feedback. Do you think that the patch is now suitable to be included in
> future kernel releases? 

That's really up to Sarah, as she is the maintainer of this driver.

How about resending it in a format that it can be applied in, and she
will take it from there?

But, at first glance, yes, it's much nicer now that you are matching on
DMI entries, thanks for taking the time to do that.

thanks,

greg k-h