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

Craig W. Nadler wrote:
> 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

Here's a slightly better version of "usbfs_port_num.patch".

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

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 22:42:22.000000000 -0400
@@ -472,8 +472,12 @@ 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;
+		/* Display the port number counting from 1 not 0. */
+		index++;
+	}
+
 	/*
 	 * So the root hub's parent is 0 and any device that is
 	 * plugged into the root hub has a parent of 0.