Hi Tianyu,
Sorry for the long long delay in reviewing this. :(
On Fri, Jan 25, 2013 at 10:29:26AM +0800, Lan Tianyu wrote:
> xhci driver divides the root hub into two logical hubs which work
> respectively for usb 2.0 and usb 3.0 devices. They are independent
> devices in the usb core. But in the ACPI table, it's one device node
> and all usb2.0 and usb3.0 ports are under it. Binding usb port with
> its acpi node needs the raw port number which is reflected in the xhci
> extended capabilities table. This patch is to add find_raw_port_number
> callback to struct hc_driver() and fill it with xhci_find_raw_port_number().
> Otherwise, call xhci_find_raw_port_number() to get real index in the HW port
> status registers instead of scanning through the xHCI roothub port array in
> the xhci_find_real_port_number().
>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
> Change since v1:
> Don't export usb_hcd_find_raw_port_number() symbol since there is no
> its user outside of usb core.
You say this is change, but the code still has it exported.
> Change since v2:
> combine patch "usb: add find_raw_port_number callback to struct
> hc_driver()" with this patch.
>
> This patchset is rebased on the usb-next tree commit 6e24777
> "usb: ehci-s5p/ohci-exynos: Fix compatible strings for the device"
>
>
> drivers/usb/core/hcd.c | 9 +++++++++
> drivers/usb/host/xhci-mem.c | 36 ++++++++----------------------------
> drivers/usb/host/xhci-pci.c | 1 +
> drivers/usb/host/xhci.c | 22 ++++++++++++++++++++++
> drivers/usb/host/xhci.h | 1 +
> include/linux/usb/hcd.h | 2 ++
> 6 files changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 2459896..774ac61 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2368,6 +2368,15 @@ int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
> }
> EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
>
> +int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
> +{
> + if (!hcd->driver->find_raw_port_number)
> + return port1;
> +
> + return hcd->driver->find_raw_port_number(hcd, port1);
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_find_raw_port_number);
> +
^^^
> static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> unsigned int irqnum, unsigned long irqflags)
> {
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 35616ff..6dc238c 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1022,44 +1022,24 @@ void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
> * is attached to (or the roothub port its ancestor hub is attached to). All we
> * know is the index of that port under either the USB 2.0 or the USB 3.0
> * roothub, but that doesn't give us the real index into the HW port status
> - * registers. Scan through the xHCI roothub port array, looking for the Nth
> - * entry of the correct port speed. Return the port number of that entry.
> + * registers. Call xhci_find_raw_port_number() to get real index.
> */
> static u32 xhci_find_real_port_number(struct xhci_hcd *xhci,
> struct usb_device *udev)
> {
> struct usb_device *top_dev;
> - unsigned int num_similar_speed_ports;
> - unsigned int faked_port_num;
> - int i;
> + struct usb_hcd *hcd;
> +
> + if (udev->speed == USB_SPEED_SUPER)
> + hcd = xhci->shared_hcd;
> + else
> + hcd = xhci->main_hcd;
>
> for (top_dev = udev; top_dev->parent && top_dev->parent->parent;
> top_dev = top_dev->parent)
> /* Found device below root hub */;
> - faked_port_num = top_dev->portnum;
> - for (i = 0, num_similar_speed_ports = 0;
> - i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
> - u8 port_speed = xhci->port_array[i];
> -
> - /*
> - * Skip ports that don't have known speeds, or have duplicate
> - * Extended Capabilities port speed entries.
> - */
> - if (port_speed == 0 || port_speed == DUPLICATE_ENTRY)
> - continue;
>
> - /*
> - * USB 3.0 ports are always under a USB 3.0 hub. USB 2.0 and
> - * 1.1 ports are under the USB 2.0 hub. If the port speed
> - * matches the device speed, it's a similar speed port.
> - */
> - if ((port_speed == 0x03) == (udev->speed == USB_SPEED_SUPER))
> - num_similar_speed_ports++;
> - if (num_similar_speed_ports == faked_port_num)
> - /* Roothub ports are numbered from 1 to N */
> - return i+1;
> - }
> - return 0;
> + return xhci_find_raw_port_number(hcd, top_dev->portnum);
> }
You've deleted a lot of code here that's designed to work around several
corner cases:
- What if there is a root port that doesn't have an entry in the
extended capabilities?
- What if we don't understand the speed description for a root port
- What if there is a root port listed twice in the extended
capabilities and it's listed as different speeds?
And I guess that's what made me put off reviewing the patch in the first
place. I wanted to take time to make sure the code was correct, and
from a first glance, it looked like you were ignoring all those
assumptions. Staring at the code now, I can see that the usb2_ports and
usb3_ports are created by skipping ports that have those issues:
static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
{
...
xhci->usb2_ports = kmalloc(sizeof(*xhci->usb2_ports)*
xhci->num_usb2_ports, flags);
if (!xhci->usb2_ports)
return -ENOMEM;
port_index = 0;
for (i = 0; i < num_ports; i++) {
if (xhci->port_array[i] == 0x03 ||
xhci->port_array[i] == 0 ||
xhci->port_array[i] == DUPLICATE_ENTRY)
continue;
xhci->usb2_ports[port_index] =
&xhci->op_regs->port_status_base +
NUM_PORT_REGS*i;
So the addresses in usb2_ports and usb3_ports are the addresses of known
good ports that we've exposed to the USB core. Any ports that are
skipped because of the reasons above won't be in those arrays.
I agree that this code is fine, and should not change any current behavior.
However, a better description of why it's safe to rip out the above code
would have been helpful.
Can you fix your EXPORT issue and resubmit this patch? I will add some
description as to why it's safe to delete the above code.
> +/*
> + * Transfer the port index into real index in the HW port status
> + * registers. Caculate offset between the port's PORTSC register
> + * and port status base. Divide the number of per port register
> + * to get the real index. The raw port number bases 1.
> + */
> +int xhci_find_raw_port_number(struct usb_hcd *hcd, int port1)
> +{
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + __le32 __iomem *base_addr = &xhci->op_regs->port_status_base;
> + __le32 __iomem *addr;
> + int raw_port;
> +
> + if (hcd->speed != HCD_USB3)
> + addr = xhci->usb2_ports[port1 - 1];
> + else
> + addr = xhci->usb3_ports[port1 - 1];
> +
> + raw_port = (addr - base_addr)/NUM_PORT_REGS + 1;
> + return raw_port;
> +}
> +
>
> #ifdef CONFIG_USB_SUSPEND
>
> /* BESL to HIRD Encoding array for USB2 LPM */
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index f791bd0..55345d9 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1829,6 +1829,7 @@ void xhci_test_and_clear_bit(struct xhci_hcd *xhci, __le32 __iomem **port_array,
> int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
> char *buf, u16 wLength);
> int xhci_hub_status_data(struct usb_hcd *hcd, char *buf);
> +int xhci_find_raw_port_number(struct usb_hcd *hcd, int port1);
>
> #ifdef CONFIG_PM
> int xhci_bus_suspend(struct usb_hcd *hcd);
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 608050b..61b88b5 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -357,6 +357,7 @@ struct hc_driver {
> */
> int (*disable_usb3_lpm_timeout)(struct usb_hcd *,
> struct usb_device *, enum usb3_link_state state);
> + int (*find_raw_port_number)(struct usb_hcd *, int);
> };
>
> extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb);
> @@ -396,6 +397,7 @@ extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd);
> extern int usb_add_hcd(struct usb_hcd *hcd,
> unsigned int irqnum, unsigned long irqflags);
> extern void usb_remove_hcd(struct usb_hcd *hcd);
> +extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
>
> struct platform_device;
> extern void usb_hcd_platform_shutdown(struct platform_device *dev);
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
