Hi Crane,
Comments inline.
I tried to apply this patch, and patches 2-4 that Libin sent, but I
could only get this one to apply to 2.6.34-rc1. I'm seeing issues with
this patch when I test with an external hub; I'll send the dmesg
separately.
Sarah Sharp
On Wed, Mar 24, 2010 at 02:42:03PM +0800, Crane Cai wrote:
> Hi Sarah,
>
> I follow your suggestions, the changes are:
>
> * Split stop endpoints into 3 functions, find slot id by port number, stop device
> with xhci command allocated, ring the device endpoint.
> * Redefine the marco for suspend bit in stop endpoint command.
> * Add some comments
>
> Suppose this one can let you say ACK. The formal one will be sent by Libin in
> series.
>
> ---
> From 814af1d249b3de5d91823492f77ca14a6d225d3b Mon Sep 17 00:00:00 2001
> From: Crane Cai <crane.cai@xxxxxxx>
> Date: Wed, 24 Mar 2010 10:46:50 +0800
> Subject: [PATCH] xHCI: port power management implementation
>
> Add software trigger USB device suspend resume function hook.
> Do port suspend & resume in terms of xHCI spec.
>
> Port Suspend:
> Stop all endpoints via Stop Endpoint Command with Suspend (SP) flag set.
> Place individual ports into suspend mode by writing '3' for Port Link State
> (PLS) field into PORTSC register. This can only be done when the port is in
> Enabled state. When writing, the Port Link State Write Strobe (LWS) bit shall
> be set to '1'.
> Allocate an xhci_command and stash it in xhci_virt_device to wait completion for
> the last Stop Endpoint Command. Use the Suspend bit in TRB to indicate the Stop
> Endpoint Command is for port suspend. Based on Sarah's suggestion.
>
> Port Resume:
> Write '0' in PLS field, device will transition to running state.
> Ring an endpoints' doorbell to restart it.
>
> System trigger suspend/resume via:
> echo suspend > /sys/bus/usb/devices/../power/level
You need to echo "auto" here, rather than suspend. "suspend" does not
work on kernels newer than 2.6.32. Using "auto" allows the kernel to
suspend the device when it's not idle, rather than forcing it into
suspend immediately. Most distros will echo auto here and let the USB
core do the work. You can't properly test your patches without setting
this to auto.
> echo on > /sys/bus/usb/devices/../power/level
Did you test remote wakeup at all? I'm seeing issues with external hubs
when you plug in a mouse when the hub is suspended (I'll send dmesg
separately).
> Tested with USB storage device & mouse.
So a USB storage device will never suspend, because the current mass
storage device driver doesn't support suspend. I think the USB HID
driver supports suspend though. The pl2303 USB-to-serial driver
supports suspend too.
You can be sure a device is really auto-suspended if you see a line like
this in the dmesg:
usb 1-3: usb auto-suspend
Did you see that in your testing?
> Ref: USB device remote wake need another patch to implement. For details of
> how USB subsystem do power management, please see:
> Documentation/usb/power-management.txt
>
> Signed-off-by: Crane Cai <crane.cai@xxxxxxx>
> ---
> drivers/usb/host/xhci-hcd.c | 2 +-
> drivers/usb/host/xhci-hub.c | 190 +++++++++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci-ring.c | 32 ++++++-
> drivers/usb/host/xhci.h | 16 +++-
> 4 files changed, 232 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hcd.c b/drivers/usb/host/xhci-hcd.c
> index 4cb69e0..da69521 100644
> --- a/drivers/usb/host/xhci-hcd.c
> +++ b/drivers/usb/host/xhci-hcd.c
> @@ -839,7 +839,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> ep->stop_cmd_timer.expires = jiffies +
> XHCI_STOP_EP_CMD_TIMEOUT * HZ;
> add_timer(&ep->stop_cmd_timer);
> - xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index);
> + xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index, 0);
> xhci_ring_cmd_db(xhci);
> }
> done:
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 208b805..ca46636 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -129,6 +129,100 @@ static u32 xhci_port_state_to_neutral(u32 state)
> return (state & XHCI_PORT_RO) | (state & XHCI_PORT_RWS);
> }
>
> +/*
> + * find slot id based on port number.
> + */
> +static int xhci_find_slot_id_by_port(struct xhci_hcd *xhci, u16 port)
> +{
> + int slot_id;
> + int i;
> + struct xhci_container_ctx *out_ctx;
> + struct xhci_slot_ctx *slot_ctx;
> +
> + slot_id = 0;
> + for (i = 0; i < MAX_HC_SLOTS; i++) {
> + if (!xhci->devs[i])
> + continue;
> + out_ctx = xhci->devs[i]->out_ctx;
> + slot_ctx = xhci_get_slot_ctx(xhci, out_ctx);
> + if (DEVINFO_TO_ROOT_HUB_PORT(slot_ctx->dev_info2) == port) {
> + slot_id = i;
> + break;
> + }
> + }
> +
> + return slot_id;
> +}
> +
> +/*
> + * Stop device
> + * It issues stop endpoint command for EP 0 to 30. And wait the last command
> + * to complete.
> + * suspend will set to 1, if suspend bit need to set in command.
> + * Require xhci spin lock and will release it to wait commands finish.
> + */
> +static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> +{
> + struct xhci_virt_device *virt_dev;
> + struct xhci_command *cmd;
> + unsigned long flags;
> + int timeleft;
> + int ret;
> + int i;
> +
> + ret = 0;
> + virt_dev = xhci->devs[slot_id];
> + cmd = xhci_alloc_command(xhci, false, true, GFP_NOIO);
> + if (!cmd) {
> + xhci_dbg(xhci, "Couldn't allocate command structure.\n");
> + return -ENOMEM;
> + }
> +
> + spin_lock_irqsave(&xhci->lock, flags);
> + for (i = 0; i < LAST_EP_INDEX; i++)
> + xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
It seems like you're issuing a stop endpoint command for all 30
endpoints in the device context, regardless of if they are actually used
by the device.
Say a device declares endpoints 0x03 and 0x86. Then the endpoint
contexts that are used will be at indexes 0, 5, and 12. But you issue a
stop endpoint command for all 30 endpoint indexes. That means you're
issuing 3 out of 30 useful commands, and waiting for all of them to
complete before suspending the device.
That seems a bit wasteful. Why not look at the device output context
and only issue the stop endpoint command if the dequeue pointer is
non-null? To make sure you enqueue a command to the command list before
issuing the last stop endpoint command, you can save the default control
endpoint (index 0) for last. All devices must have that endpoint, so it
should be fine.
> + cmd->command_trb = xhci->cmd_ring->enqueue;
> + list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
> + xhci_queue_stop_endpoint(xhci, slot_id, LAST_EP_INDEX, suspend);
> + xhci_ring_cmd_db(xhci);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +
> + /* Wait for last stop endpoint command to finish */
> + timeleft = wait_for_completion_interruptible_timeout(
> + cmd->completion,
> + USB_CTRL_SET_TIMEOUT);
> + if (timeleft <= 0) {
> + xhci_warn(xhci, "%s while waiting for reset device command\n",
> + timeleft == 0 ? "Timeout" : "Signal");
> + spin_lock_irqsave(&xhci->lock, flags);
> + /* The timeout might have raced with the event ring handler, so
> + * only delete from the list if the item isn't poisoned.
> + */
> + if (cmd->cmd_list.next != LIST_POISON1)
> + list_del(&cmd->cmd_list);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + ret = -ETIME;
> + goto command_cleanup;
> + }
> +
> +command_cleanup:
> + xhci_free_command(xhci, cmd);
> + return ret;
> +}
> +
> +/*
> + * Ring device, it rings the all doorbells unconditionally.
> + */
> +static void xhci_ring_device(struct xhci_hcd *xhci, int slot_id)
> +{
> + int i;
> +
> + for (i = 0; i < LAST_EP_INDEX + 1; i++)
> + ring_ep_doorbell(xhci, slot_id, i);
> +
> + return;
> +}
> +
Please only ring the doorbells for endpoints that are actively being
used. Hosts are supposed to ignore endpoints that are disabled, but I
could see some buggy hosts dereferencing NULL dequeue pointers when the
doorbell is rung.
> static void xhci_disable_port(struct xhci_hcd *xhci, u16 wIndex,
> u32 __iomem *addr, u32 port_status)
> {
> @@ -162,6 +256,10 @@ static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue,
> status = PORT_PEC;
> port_change_bit = "enable/disable";
> break;
> + case USB_PORT_FEAT_C_SUSPEND:
> + status = PORT_PLC;
> + port_change_bit = "suspend/resume";
> + break;
> default:
> /* Should never happen */
> return;
> @@ -182,6 +280,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> u32 temp, status;
> int retval = 0;
> u32 __iomem *addr;
> + int slot_id;
>
> ports = HCS_MAX_PORTS(xhci->hcs_params1);
>
> @@ -211,9 +310,18 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> if ((temp & PORT_OCC))
> status |= 1 << USB_PORT_FEAT_C_OVER_CURRENT;
> /*
> - * FIXME ignoring suspend, reset, and USB 2.1/3.0 specific
> + * FIXME ignoring reset and USB 2.1/3.0 specific
> * changes
> */
> + if ((temp & PORT_PLS_MASK) == XDEV_U3
> + && (temp & PORT_POWER))
> + status |= 1 << USB_PORT_FEAT_SUSPEND;
> + if ((temp & PORT_PLS_MASK) == XDEV_U0
> + && (temp & PORT_POWER)
> + && test_bit(wIndex, &xhci->suspended_ports)) {
> + clear_bit(wIndex, &xhci->suspended_ports);
> + set_bit(wIndex, &xhci->port_c_suspend);
> + }
> if (temp & PORT_CONNECT) {
> status |= 1 << USB_PORT_FEAT_CONNECTION;
> status |= xhci_port_speed(temp);
> @@ -226,6 +334,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> status |= 1 << USB_PORT_FEAT_RESET;
> if (temp & PORT_POWER)
> status |= 1 << USB_PORT_FEAT_POWER;
> + if (test_bit(wIndex, &xhci->port_c_suspend))
> + status |= 1 << USB_PORT_FEAT_C_SUSPEND;
> xhci_dbg(xhci, "Get port status returned 0x%x\n", status);
> put_unaligned(cpu_to_le32(status), (__le32 *) buf);
> break;
> @@ -238,6 +348,41 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> temp = xhci_readl(xhci, addr);
> temp = xhci_port_state_to_neutral(temp);
> switch (wValue) {
> + case USB_PORT_FEAT_SUSPEND:
> + temp = xhci_readl(xhci, addr);
> + /* In spec software should not attempt to suspend
> + * a port unless the port reports that it is in the
> + * enabled (PED = ‘1’,PLS < ‘3’) state.
> + */
> + if ((temp & PORT_PE) == 0 || (temp & PORT_RESET)
> + || (temp & PORT_PLS_MASK) >= XDEV_U3) {
> + xhci_dbg(xhci, "goto error status = 0x%x\n",
> + temp);
> + goto error;
> + }
> +
> + slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
> + if (!slot_id) {
> + xhci_dbg(xhci, "slot_id is zero\n");
> + goto error;
> + }
> + /* unlock to execute stop endpoint commands */
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + xhci_stop_device(xhci, slot_id, 1);
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_U3;
> + xhci_writel(xhci, temp, addr);
> +
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + msleep(10); /* wait device to enter */
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_readl(xhci, addr);
> + set_bit(wIndex, &xhci->suspended_ports);
> + break;
> case USB_PORT_FEAT_POWER:
> /*
> * Turn on ports, even if there isn't per-port switching.
> @@ -271,6 +416,49 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> temp = xhci_readl(xhci, addr);
> temp = xhci_port_state_to_neutral(temp);
> switch (wValue) {
> + case USB_PORT_FEAT_SUSPEND:
> + temp = xhci_readl(xhci, addr);
> + xhci_dbg(xhci, "clear USB_PORT_FEAT_SUSPEND\n");
> + xhci_dbg(xhci, "PORTSC %04x\n", temp);
> + if (temp & PORT_RESET)
> + goto error;
> + if (temp & XDEV_U3) {
> + if ((temp & PORT_PE) == 0)
> + goto error;
> + if (DEV_SUPERSPEED(temp)) {
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_U0;
> + xhci_writel(xhci, temp, addr);
> + } else {
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_RESUME;
> + xhci_writel(xhci, temp, addr);
> +
> + spin_unlock_irqrestore(&xhci->lock,
> + flags);
> + msleep(20);
> + spin_lock_irqsave(&xhci->lock, flags);
> +
> + temp = xhci_readl(xhci, addr);
> + temp = xhci_port_state_to_neutral(temp);
> + temp &= ~PORT_PLS_MASK;
> + temp |= PORT_LINK_STROBE | XDEV_U0;
> + xhci_writel(xhci, temp, addr);
> + }
> + set_bit(wIndex, &xhci->port_c_suspend);
> + }
> +
> + slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
> + if (!slot_id) {
> + xhci_dbg(xhci, "slot_id is zero\n");
> + goto error;
> + }
> + xhci_ring_device(xhci, slot_id);
> + break;
> + case USB_PORT_FEAT_C_SUSPEND:
> + clear_bit(wIndex, &xhci->port_c_suspend);
> case USB_PORT_FEAT_C_RESET:
> case USB_PORT_FEAT_C_CONNECTION:
> case USB_PORT_FEAT_C_OVER_CURRENT:
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 6ba841b..7529f34 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -292,7 +292,7 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
> xhci_readl(xhci, &xhci->dba->doorbell[0]);
> }
>
> -static void ring_ep_doorbell(struct xhci_hcd *xhci,
> +void ring_ep_doorbell(struct xhci_hcd *xhci,
> unsigned int slot_id,
> unsigned int ep_index)
> {
> @@ -942,7 +942,26 @@ bandwidth_change:
> complete(&xhci->addr_dev);
> break;
> case TRB_TYPE(TRB_STOP_RING):
> - handle_stopped_endpoint(xhci, xhci->cmd_ring->dequeue);
> + /*
> + * When suspend bit set, no other action executed, esp do not
> + * ring door bell in handle_stopped_endpoint.
> + * When suspend port called, USB core has flushed endpoint.
> + * There are no TDs on canclellation list.
> + */
> + if (unlikely(TRB_TO_SUSPEND_PORT(
> + xhci->cmd_ring->dequeue->generic.field[3]))) {
> + slot_id = TRB_TO_SLOT_ID(
> + xhci->cmd_ring->dequeue->generic.field[3]);
> + virt_dev = xhci->devs[slot_id];
> + if (virt_dev)
> + handle_cmd_in_cmd_wait_list(xhci, virt_dev,
> + event);
> + else
> + xhci_warn(xhci, "Stop endpoint command "
> + "completion for disabled slot %u\n",
> + slot_id);
> + } else
> + handle_stopped_endpoint(xhci, xhci->cmd_ring->dequeue);
Can you please put this code into handle_stopped_endpoint instead of
sticking it in the switch statement? I want to keep that switch
statement as small as possible, because it's getting really ugly.
Sarah Sharp
--
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
