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
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);
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;
-------------------------------------------------------------------------
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/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
