Date:	Thu, 24 Jul 2008 13:14:50 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Yinghai Lu <yhlu.kernel@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Arjan van de Ven <arjan@...radead.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Greg KH <greg@...ah.com>, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org
Subject: Re: [PATCH] x86: usb debug port early console v3


very nice feature!

The code structure looks good to me, here's a few minor style nits:

> +	/* Now that we have observed the completed transaction,
> +	 * clear the done bit.
> +	 */

while i understand that this is cut & pasted code, please use standard 
comment style:

  /*
   * Comment ...
   *  ... line.
   */

(ditto the same mistake in other places too)

> +	/* Read the result */
> +	ret = dbgp_bulk_read(devnum, 0, data, size);
> +	return ret;

do:

    return dbgp_bulk_read(devnum, 0, data, size);

> +	if (!(read_pci_config_16(num, slot, func, PCI_STATUS) &
> +		PCI_STATUS_CAP_LIST))
> +		return 0;
> +	pos = read_pci_config_byte(num, slot, func, PCI_CAPABILITY_LIST);

it's generally nicer to the eyes to add an extra newline after a block 
with return in it.

> +	for (bytes = 0; bytes < 48 && pos >= 0x40; bytes++) {
> +		u8 id;
> +		pos &= ~3;

please put a newline between variable definitions and first statement.

> +
> +static __u32 __init find_dbgp(int ehci_num, unsigned *rbus, unsigned *rslot,
> +				unsigned *rfunc)
> +{
> +	unsigned bus, slot, func;
> +
> +	for (bus = 0; bus < 256; bus++) {
> +		for (slot = 0; slot < 32; slot++) {
> +			for (func = 0; func < 8; func++) {
> +				u32 class;
> +				unsigned cap;
> +
> +				class = read_pci_config(bus, slot, func,
> +							 PCI_CLASS_REVISION);
> +				if ((class >> 8) != PCI_CLASS_SERIAL_USB_EHCI)
> +					continue;
> +				cap = find_cap(bus, slot, func,
> +						 PCI_CAP_ID_EHCI_DEBUG);

the line 80 breaks you had to add here show that the nesting is too deep 
here - i'd suggest a helper __find_dbgp() function to put the iterator 
into.

> +	if ((ctrl & DBGP_CLAIM) != DBGP_CLAIM) {
> +		dbgp_printk("No device in debug port\n");
> +		writel(ctrl & ~DBGP_CLAIM, &ehci_debug->control);
> +		return -1;
> +
> +	}

stray newline.

> +static int __init early_dbgp_init(char *s)
> +{
> +	struct usb_debug_descriptor dbgp_desc;
> +	void __iomem *ehci_bar;
> +	unsigned ctrl, devnum;
> +	unsigned bus, slot, func, cap;
> +	unsigned debug_port, bar, offset;
> +	unsigned bar_val;
> +	char *e;
> +	int ret;
> +	unsigned dbgp_num;

use an explicit integer type please instead of 'unsigned'. Also, try to 
use reverse christmas-tree ordering for same-type entries (and where 
possible, between different types as well):

> +	struct usb_debug_descriptor dbgp_desc;
> +	unsigned int debug_port, bar, offset;
> +	unsigned int bus, slot, func, cap;
> +	unsigned int ctrl, devnum;
> +	unsigned int dbgp_num;
> +	unsigned int bar_val;
> +	void __iomem *ehci_bar;
> +	char *e;
> +	int ret;


here:

> +	dbgp_num = 0;
> +	if (*s)
> +		dbgp_num = simple_strtoul(s, &e, 10);
> +	dbgp_printk("dbgp_num: %d\n", dbgp_num);
> +	cap = find_dbgp(dbgp_num, &bus, &slot, &func);
> +	if (!cap)
> +		return -1;
> +
> +	dbgp_printk("Found EHCI debug port\n");

i'd suggest a newline after the first dbgp_printk(), to make the two 
sections stand out better.

> +	}
> +
> +

stray newline.

> +	/* FIXME I don't have the bar size so just guess PAGE_SIZE is more
> +	 * than enough.  1K is the biggest I have seen.
> +	 */

comment style.

> +	ret = ehci_setup();
> +	if (ret < 0) {
> +		dbgp_printk("ehci_setup failed\n");
> +		ehci_debug = 0;
> +		return -1;

please put newlines before return statements, to make sure there's a 
hickup in the visual flow during review. (which hickup return statements 
should cause, they must not be glossed over)

> +	}
> +
> +

stray newline.

> Index: linux-2.6/drivers/usb/host/ehci.h
> ===================================================================
> --- linux-2.6.orig/drivers/usb/host/ehci.h
> +++ linux-2.6/drivers/usb/host/ehci.h
> @@ -210,146 +210,11 @@ timer_action (struct ehci_hcd *ehci, enu

i suggest you make this code movement a separate patch. In the unlikely 
event of there being any regression it's an easier bisection target.

looks good to me otherwise.

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