linux-kernel - Re: [PATCH] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware

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: <20120801233215.GD7564@xanatos>
Date:	Wed, 1 Aug 2012 16:32:15 -0700
From:	Sarah Sharp <sarah.a.sharp@...ux.intel.com>
To:	"Alexis R. Cortes" <alexis.cortes@...com>
Cc:	'Peter Stuge' <peter@...ge.se>, gregkh@...uxfoundation.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	brian.quach@...com, jorge.llamas@...com
Subject: Re: [PATCH] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP
 Hardware

On Wed, Aug 01, 2012 at 04:46:27PM -0500, Alexis R. Cortes wrote:
> Hi Sarah,
> 
> Sure!! I'll update the patch's description and will send another patch in a
> few moments.
> 
> As an additional comment, I ran the 'checkpatch.pl' script and verified
> there were no errors before submitting the patch. I only got a few Warnings
> related to the line limits. 

Can you break the code up a bit so the line limit warnings don't appear?
The long line warnings are really indicative of a deeper problem,
usually that the parent function has gotten much too long, and should be
broken into smaller functions.

The only exception to the long line rule (and one that checkpatch
doesn't usually complain about) is that we don't break up lines in the
middle of strings.  It makes searching for the strings later much
harder.  So either leave the really long string line, or break it up
into two xhci_dbg statements.

In general, warnings from checkpatch.pl stop my patch application and
submission work flow, so I don't accept patches with warnings.

For instance:

			if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
				if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1)) {
					if ((temp & PORT_PLS_MASK) == XDEV_U0) {
						xhci->port_status_u0 |= 1 << wIndex;
						if (xhci->port_status_u0 == ((1 << xhci->num_usb3_ports)-1)) {
							del_timer_sync(&xhci->comp_mode_recovery_timer);
							xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted. "
                                                                       "All USB3 ports have entered U0 at least once.\n");
                                               }
                                       }
                               }
                       }

Could be broken out into a new function, like so:

void xhci_del_comp_mod_timer(struct xhci_hcd *xhci)
{
	unsigned int all_ports_seen_u0 = ((1 << xhci->num_usb3_ports)-1));
	boolean this_port_in_u0 = ((temp & PORT_PLS_MASK) == XDEV_U0);

	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK))
		return;

	if (xhci->port_status_u0 != all_ports_seen_u0 && this_port_in_u0) {
		xhci->port_status_u0 |= 1 << wIndex;
		if (xhci->port_status_u0 == all_ports_seen_u0) {
			del_timer_sync(&xhci->comp_mode_recovery_timer);
			xhci_dbg(xhci, "All USB3 ports have entered U0 at least once.\n");
			xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted.\n");
		}
	}
}

Then you can call that new function in the ugly long indent-heavy function
in xhci-hub.c.

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