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

"Craig W. Nadler" <[email protected]>
Newsgroups gmane.linux.usb.devel
Message-ID <[email protected]> 
Alan Stern wrote:
> 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.
>   

Sorry about that, I think I have that fixed now.

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

Thanks, it's been fixed.

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

Thanks, it's been fixed.

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

Thanks, it's been fixed.

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

After thinking about it, it seems very scary to unnecessarily change so 
many HCDs. Especially as I have no way to test many of them. I've 
attached a patch for just the EHCI driver.

I also noticed that the port numbers are shown incorrectly for devices 
in /proc/bus/usb/devices . It has the ports numbered from 0 instead of 1 
as specified in the USB spec.. The attached patch called 
"usbfs_port_num.patch" adds one to the 0 based port number before 
displaying it. Please note that Host Controller still has a port number 
of 0.

Best Regards,

Craig Nadler

-------------------------------------------------------------------------
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
ehci_wIndex.patch (text/x-patch, 5.2 KB) 
diff -uprN a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
--- a/drivers/usb/host/ehci-hub.c	2007-06-07 17:27:31.000000000 -0400
+++ b/drivers/usb/host/ehci-hub.c	2007-06-11 18:28:30.000000000 -0400
@@ -444,11 +444,12 @@ 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];
+	unsigned	port_num = (wIndex & 0xFF) - 1;
+	unsigned	selector = (wIndex >> 8) & 0xFF;
+	u32 __iomem	*status_reg = &ehci->regs->port_status[port_num];
 	u32		temp, status;
 	unsigned long	flags;
 	int		retval = 0;
-	unsigned	selector;
 
 	/*
 	 * FIXME:  support SetPortFeatures USB_PORT_FEAT_INDICATOR.
@@ -470,9 +471,8 @@ static int ehci_hub_control (
 		}
 		break;
 	case ClearPortFeature:
-		if (!wIndex || wIndex > ports)
+		if (port_num >= ports)
 			goto error;
-		wIndex--;
 		temp = ehci_readl(ehci, status_reg);
 
 		/*
@@ -502,7 +502,7 @@ static int ehci_hub_control (
 				temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
 				ehci_writel(ehci, temp | PORT_RESUME,
 						status_reg);
-				ehci->reset_done [wIndex] = jiffies
+				ehci->reset_done[port_num] = jiffies
 						+ msecs_to_jiffies (20);
 			}
 			break;
@@ -541,9 +541,8 @@ static int ehci_hub_control (
 		//cpu_to_le32s ((u32 *) buf);
 		break;
 	case GetPortStatus:
-		if (!wIndex || wIndex > ports)
+		if (port_num >= ports)
 			goto error;
-		wIndex--;
 		status = 0;
 		temp = ehci_readl(ehci, status_reg);
 
@@ -559,20 +558,20 @@ static int ehci_hub_control (
 		if (temp & PORT_RESUME) {
 
 			/* Remote Wakeup received? */
-			if (!ehci->reset_done[wIndex]) {
+			if (!ehci->reset_done[port_num]) {
 				/* resume signaling for 20 msec */
-				ehci->reset_done[wIndex] = jiffies
+				ehci->reset_done[port_num] = jiffies
 						+ msecs_to_jiffies(20);
 				/* check the port again */
 				mod_timer(&ehci_to_hcd(ehci)->rh_timer,
-						ehci->reset_done[wIndex]);
+						ehci->reset_done[port_num]);
 			}
 
 			/* resume completed? */
 			else if (time_after_eq(jiffies,
-					ehci->reset_done[wIndex])) {
+					ehci->reset_done[port_num])) {
 				status |= 1 << USB_PORT_FEAT_C_SUSPEND;
-				ehci->reset_done[wIndex] = 0;
+				ehci->reset_done[port_num] = 0;
 
 				/* stop resume signaling */
 				temp = ehci_readl(ehci, status_reg);
@@ -584,7 +583,7 @@ static int ehci_hub_control (
 				if (retval != 0) {
 					ehci_err(ehci,
 						"port %d resume error %d\n",
-						wIndex + 1, retval);
+						port_num + 1, retval);
 					goto error;
 				}
 				temp &= ~(PORT_SUSPEND|PORT_RESUME|(3<<10));
@@ -594,9 +593,9 @@ static int ehci_hub_control (
 		/* whoever resets must GetPortStatus to complete it!! */
 		if ((temp & PORT_RESET)
 				&& time_after_eq(jiffies,
-					ehci->reset_done[wIndex])) {
+					ehci->reset_done[port_num])) {
 			status |= 1 << USB_PORT_FEAT_C_RESET;
-			ehci->reset_done [wIndex] = 0;
+			ehci->reset_done[port_num] = 0;
 
 			/* force reset to complete */
 			ehci_writel(ehci, temp & ~(PORT_RWC_BITS | PORT_RESET),
@@ -608,22 +607,22 @@ static int ehci_hub_control (
 					PORT_RESET, 0, 750);
 			if (retval != 0) {
 				ehci_err (ehci, "port %d reset error %d\n",
-					wIndex + 1, retval);
+					port_num + 1, retval);
 				goto error;
 			}
 
 			/* see what we found out */
-			temp = check_reset_complete (ehci, wIndex, status_reg,
+			temp = check_reset_complete (ehci, port_num, status_reg,
 					ehci_readl(ehci, status_reg));
 		}
 
 		/* transfer dedicated ports to the companion hc */
 		if ((temp & PORT_CONNECT) &&
-				test_bit(wIndex, &ehci->companion_ports)) {
+				test_bit(port_num, &ehci->companion_ports)) {
 			temp &= ~PORT_RWC_BITS;
 			temp |= PORT_OWNER;
 			ehci_writel(ehci, temp, status_reg);
-			ehci_dbg(ehci, "port %d --> companion\n", wIndex + 1);
+			ehci_dbg(ehci, "port %d --> companion\n", port_num + 1);
 			temp = ehci_readl(ehci, status_reg);
 		}
 
@@ -652,7 +651,7 @@ static int ehci_hub_control (
 #ifndef	EHCI_VERBOSE_DEBUG
 	if (status & ~0xffff)	/* only if wPortChange is interesting */
 #endif
-		dbg_port (ehci, "GetStatus", wIndex + 1, temp);
+		dbg_port (ehci, "GetStatus", port_num + 1, temp);
 		put_unaligned(cpu_to_le32 (status), (__le32 *) buf);
 		break;
 	case SetHubFeature:
@@ -666,11 +665,8 @@ static int ehci_hub_control (
 		}
 		break;
 	case SetPortFeature:
-		selector = wIndex >> 8;
-		wIndex &= 0xff;
-		if (!wIndex || wIndex > ports)
+		if (port_num >= ports)
 			goto error;
-		wIndex--;
 		temp = ehci_readl(ehci, status_reg);
 		if (temp & PORT_OWNER)
 			break;
@@ -704,10 +700,11 @@ static int ehci_hub_control (
 					&& PORT_USB11 (temp)) {
 				ehci_dbg (ehci,
 					"port %d low speed --> companion\n",
-					wIndex + 1);
+					port_num + 1);
 				temp |= PORT_OWNER;
 			} else {
-				ehci_vdbg (ehci, "port %d reset\n", wIndex + 1);
+				ehci_vdbg (ehci, "port %d reset\n",
+						port_num + 1);
 				temp |= PORT_RESET;
 				temp &= ~PORT_PE;
 
@@ -715,7 +712,7 @@ static int ehci_hub_control (
 				 * caller must wait, then call GetPortStatus
 				 * usb 2.0 spec says 50 ms resets on root
 				 */
-				ehci->reset_done [wIndex] = jiffies
+				ehci->reset_done[port_num] = jiffies
 						+ msecs_to_jiffies (50);
 			}
 			ehci_writel(ehci, temp, status_reg);
usbfs_port_num.patch (text/x-patch, 1.2 KB) 
diff -uprN a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
--- a/drivers/usb/core/devices.c	2007-06-07 17:27:31.000000000 -0400
+++ b/drivers/usb/core/devices.c	2007-06-11 21:12:23.000000000 -0400
@@ -460,6 +460,7 @@ static ssize_t usb_device_dump(char __us
 	int parent_devnum = 0;
 	char *pages_start, *data_end, *speed;
 	unsigned int length;
+	unsigned int port_num = 0;
 	ssize_t total_written = 0;
 	
 	/* don't bother with anything else if we're not writing any data */
@@ -472,8 +473,10 @@ static ssize_t usb_device_dump(char __us
         if (!(pages_start = (char*) __get_free_pages(GFP_KERNEL,1)))
                 return -ENOMEM;
 		
-	if (usbdev->parent && usbdev->parent->devnum != -1)
+	if (usbdev->parent && usbdev->parent->devnum != -1) {
 		parent_devnum = usbdev->parent->devnum;
+		port_num = index + 1;
+	}
 	/*
 	 * So the root hub's parent is 0 and any device that is
 	 * plugged into the root hub has a parent of 0.
@@ -491,7 +494,7 @@ static ssize_t usb_device_dump(char __us
 	}
 	data_end = pages_start + sprintf(pages_start, format_topo,
 			bus->busnum, level, parent_devnum,
-			index, count, usbdev->devnum,
+			port_num, count, usbdev->devnum,
 			speed, usbdev->maxchild);
 	/*
 	 * level = topology-tier level;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.