usb: host: xhci: Compliance Mode port recovery
A message that is not in the archive (sent off-list or not yet received): 002801cd4e68$a4b4f3a0$ee1edae0$@[email protected]
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.
Quoted 3 lines · Alexis Cortes
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.
Quoted 14 lines
Signed-off-by Alexis R. Cortes <[email protected]>--- drivers/usb/host/xhci.c | 39 +++++++++++++++++++++++++++++++++++++++ drivers/usb/host/xhci.h | 4 ++++ 2 files changed, 43 insertions(+), 0 deletions(-)drivers/usb/host/xhci.c +0
-0@@ -397,6 +397,39 @@ static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)#endif
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 409
Quoted 10 lines
drivers/usb/host/xhci.c +10
-0+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.
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 415
Quoted 6 lines
drivers/usb/host/xhci.c +6
-0+ 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.
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 420
Quoted 5 lines
drivers/usb/host/xhci.c +4
-0+ } + } +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?
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 433
Quoted 13 lines
drivers/usb/host/xhci.c +13
-0+} + +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.
Quoted 14 lines
drivers/usb/host/xhci.c +0
-0/* * Initialize memory for HCD and xHC (one-time init). *
drivers/usb/host/xhci.c +3
-0@@ -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; }
drivers/usb/host/xhci.c void xhci_stop(struct usb_hcd *hcd) · line 669
Quoted 6 lines
drivers/usb/host/xhci.c +3
-0@@ -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.
Quoted 8 lines
drivers/usb/host/xhci.c +0
-0if (xhci->quirks &XHCI_AMD_PLL_FIX )usb_amd_dev_put ();drivers/usb/host/xhci.h +0
-0
drivers/usb/host/xhci.h struct xhci_hcd { · line 1512
Quoted 7 lines
drivers/usb/host/xhci.h +4
-0@@ -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
A message that is not in the archive (sent off-list or not yet received): 004101cd4f33$755e8eb0$601bac10$@[email protected]
Quoted 7 lines · Alexis Cortes
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?
Quoted 8 lines
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"?
Quoted 5 lines
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.
Quoted 3 lines
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/
Quoted 8 lines
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.
Quoted 24 lines
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 5 lines
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.ccode that checks for port changes, you should fakea
Quoted 1 line
link status change. The USB core will notice the status change and see
the
Quoted 1 line
port's link status of compliance. Then the USB core will take care of
issuing
Quoted 5 lines
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
Quoted 6 lines
Quoted 2 lines
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
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
drivers/usb/host/xhci.h index renamed from drivers/usb/host/xhci.h +0
-0de3d6e3..0f11cb8 100644
drivers/usb/host/xhci.h +0
-0@@ -1506,6 +1506,10 @@ struct xhci_hcd { unsigned sw_lpm_support:1;Quoted 8 lines
drivers/usb/host/xhci.h struct xhci_hcd { · line 1512
Quoted 6 lines
drivers/usb/host/xhci.h +4
-0/* 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 2How often do you really need this timer to run? Do you really want to
Quoted 8 lines
wake
Quoted 2 lines
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
Quoted 1 line
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.
Quoted 4 lines
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
Quoted 4 lines · Sarah Sharp
Quoted 3 lines
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.
Quoted 8 lines
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
A message that is not in the archive (sent off-list or not yet received): 003001cd4fd3$a7648950$f62d9bf0$@[email protected]
Quoted 12 lines · Alexis Cortes
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.
Quoted 3 lines
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
Quoted 36 lines
Quoted 5 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Wednesday, June 20, 2012 7:33 PM
To: Sarah Sharp
Cc: Alexis Cortes; [email protected];Quoted 29 lines
'Quach, Brian'; 'Llamas, Jorge'
Subject: Re: [PATCH] usb: host: xhci: Compliance Mode port recoveryQuoted 3 lines · Sarah Sharp
Quoted 3 lines
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.
Quoted 9 lines
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
Quoted 28 lines · Sarah Sharp
Quoted 12 lines · Alexis Cortes
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.
Quoted 3 lines
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.
Quoted 7 lines
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
Quoted 8 lines · Greg KH
Quoted 3 lines · Sarah Sharp
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.
Quoted 5 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
Quoted 3 lines · Alexis Cortes
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
A message that is not in the archive (sent off-list or not yet received): 003701cd4fde$fd969290$f8c3b7b0$@[email protected]
Quoted 13 lines · Alexis Cortes
Hi Sarah,
Quoted 1 line
How often is this timing issue hit? On every plug in? Every 100 plug
ins?
Quoted 2 lines
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).
Quoted 4 lines
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.
Quoted 10 lines
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
Quoted 7 lines · Sarah Sharp
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 16 lines · Alexis Cortes
Quoted 5 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Friday, June 22, 2012 11:47 AM
To: Sarah Sharp
Cc: Alexis Cortes; [email protected];Quoted 7 lines
'Quach, Brian'; 'Llamas, Jorge'
Subject: Re: [PATCH] usb: host: xhci: Compliance Mode port recoveryQuoted 3 lines · Sarah Sharp
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!"
Quoted 2 lines
Quoted 2 lines
"Uhhh..."
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 6 lines
Quoted 2 lines
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Alexis Cortes
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:
Quoted 6 lines
* Changed #define
COMP_MODE_RCVRY_TIMEOUT2 by #defineCOMP_MODE_RCVRY_MSECS2000.
* 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:
Quoted 1 line
[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?
Review
Quoted 4 lines
(quoted diff) +4
-0+ } 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?
Quoted 6 lines
(quoted diff) +1
-0+ xhci_dbg(xhci, "Compliance ModeRecovery Timer "
(quoted diff) +1
-0+ "Deleted. All USB3ports have "
(quoted diff) +1
-0+ "entered U0 at leastonce.\n");
Keep the string all on one line.
Quoted 1 line
+/*Compliance Mode Recovery Patch*/
Why is this comment needed?
Quoted 1 line
+static void
compliance_mode_rcvry(unsigned long arg)
Vowels are free, please use them :)
Review
Quoted 7 lines
(quoted diff) +7
-0+{ + struct xhci_hcd *xhci; + struct usb_hcd *hcd; + u32 temp; + int i; + + xhci = (struct xhci_hcd *) arg;
No space needed before "arg".
Review
Quoted 6 lines
(quoted diff) +6
-0+ + 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. */
Review
Quoted 3 lines
(quoted diff) +1
-0+ xhci_dbg(xhci, "Compliance Mode Detected on port %d!"
(quoted diff) +1
-0+ "Attempting recovery routine.\n", i
Don't spread strings across lines, it makes it harder to grep for them.
Review
Quoted 9 lines
(quoted diff) +9
-0+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.
Review int xhci_init(struct usb_hcd *hcd) · line 468
Quoted 13 lines
(quoted diff) +6
-0+ 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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · Alexis Cortes
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.
Quoted 7 lines
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