Re: [PATCH] x86: usb debug port early console v3

From: Ingo Molnar
Date: Thu Jul 24 2008 - 07:15:29 EST



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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/