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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20120711150604.GB22382@kroah.com>
Date:	Wed, 11 Jul 2012 08:06:04 -0700
From:	'Greg KH' <gregkh@...uxfoundation.org>
To:	Alexis Cortes <alexis.cortes@...com>
Cc:	'Sarah Sharp' <sarah.a.sharp@...ux.intel.com>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	"'Quach, Brian'" <brian.quach@...com>,
	"'Llamas, Jorge'" <jorge.llamas@...com>
Subject: Re: [PATCH] usb: host: xhci: Compliance Mode port recovery

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