Re: [PATCH] USB: ehci_hub_control() (Kernel 2.6.21.4)

On Sun, 10 Jun 2007, Craig W. Nadler wrote:

> >> --- a/drivers/usb/host/ehci-hub.c 2007-06-07 17:27:31.000000000 -0400
> >> +++ b/drivers/usb/host/ehci-hub.c 2007-06-10 13:45:14.000000000 -0400
> >> @@ -444,7 +444,8 @@ static int ehci_hub_control (
> >> ) {
> >> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> >> int ports = HCS_N_PORTS (ehci->hcs_params);
> >> - u32 __iomem *status_reg = &ehci->regs->port_status[wIndex - 1];
> >> + u8 port_num = wIndex&0xFF;
> >> + u32 __iomem *status_reg = &ehci->regs->port_status[port_num - 1];
> >> u32 temp, status;
> >> unsigned long flags;
> >> int retval = 0;
> >>     
> >
> >
> > I have some comments on the contents of this patch -- others may
> > comment on its formatting (or lack thereof!).
> >   
> Ok, I'm not sure I know what you're referring to as far as formatting. 
> If there is something I'm doing wrong please let me know.

I was referring to the fact that your email client had stripped out all 
the tab characters.  The latest version of the patch doesn't have this 
problem.

> Please note that the new patch is untested. I'm posting it to get comments.

Okay, here's a few.  On the whole I like it but it could be improved a 
little.  To start with, the formatting here is wrong:

> +	unsigned	port_num = wIndex&0xFF;

There should be spaces around the '&'.  This occurs many times.


There also are lots of places where the code follows this pattern:

> -		if (!wIndex || wIndex > ports)
> +		if (!port_num || port_num > ports)
>  			goto error;
> -		wIndex--;
> +		port_num--;

You could make things much simpler by doing it this way instead:

+	unsigned	port_num = (wIndex & 0xFF) - 1;
...
-		if (!wIndex || wIndex > ports)
+		if (port_num >= ports)
 			goto error;
-		wIndex--;


Then this sort of thing occurs often in your patch:

-				ehci->reset_done [wIndex] = jiffies
+				ehci->reset_done [port_num] = jiffies

David's preferred programming style puts a space before an open
bracket (also before an open paren when it is for a function call),
but the preferred style for the kernel is to omit such spaces.
Slowly, piece by piece, we've been removing them.  Hence when you
change a line that has the extra space in it, like here, you should
remove the space.


In uhci-hub.c:

@@ -304,7 +304,7 @@ static int uhci_hub_control(struct usb_h
 
 		if (wPortChange)
 			dev_dbg(uhci_dev(uhci), "port %d portsc %04x,%02x\n",
-					wIndex, status, lstatus);
+					port, status, lstatus);
 
 		*(__le16 *)buf = cpu_to_le16(wPortStatus);
 		*(__le16 *)(buf + 2) = cpu_to_le16(wPortChange);

Your search-and-replace strategy let you down; you need to specify
port + 1 instead of port.


Finally, having put you to all this trouble, I realized that it wasn't 
necessary.  The upper bits of wIndex are nonzero only for the 
PORT_FEAT_INDICATOR and PORT_FEAT_TEST features, and ehci-hcd is the 
only driver to support either of those.  Hence it is the only driver is 
need of this change.  However I think it's not a bad idea to update
every driver anyhow.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel