On Thu, Mar 04, 2010 at 07:21:05PM +0800, Crane Cai wrote:
> Hi Sarah,
>
> It is the modified one. And smoke tested, I add a parameter to set SP bit, and a
> variable "last_ep" to trigger complete call.
Sorry for the delay in reviewing this; I was at a conference last week.
I have two questions about the functionality of the patch, and the rest
of the comments are just nit-picky.
Sarah Sharp
> ---
> From: Crane Cai <crane.cai@xxxxxxx>
>
> drivers/usb/host/xhci-hcd.c | 2 +-
> drivers/usb/host/xhci-hub.c | 145 +++++++++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci-mem.c | 1 +
> drivers/usb/host/xhci-ring.c | 12 +++-
> drivers/usb/host/xhci.h | 17 ++++-
> 5 files changed, 170 insertions(+), 7 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..fce04e9 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -128,6 +128,69 @@ static u32 xhci_port_state_to_neutral(u32 state)
> /* Save read-only status and port state */
> return (state & XHCI_PORT_RO) | (state & XHCI_PORT_RWS);
> }
> +/*
> + * stop endpoints before the port suspend, and ring the bell before resume.
> + * caller need xhci lock and may release it to finish commands.
> + */
> +static u32 xhci_stop_endpoints(struct xhci_hcd *xhci, u16 port, int stop,
> + unsigned long *flags)
Can you please break this up into three functions? One to find the slot
ID for that port, one to stop endpoints, and one to restart them.
It's confusing to have one function do both the stopping and starting.
> +{
> + int i, slot_id;
> + struct xhci_container_ctx *out_ctx;
> + struct xhci_slot_ctx *slot_ctx;
> + struct xhci_ep_ctx *ep_ctx;
> + struct xhci_virt_ep *ep;
> +
> + slot_id = 0;
> + if (!xhci->devs)
> + return 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 (GET_SLOT_STATE(slot_ctx->dev_state) == 3 &&
Please add a #define to xhci.h to explain what "3" means for the slot
state, if you still use it after my comments below. In general, you
shouldn't hard-code register values, because people need to know what
you meant at a glance and those values could change based on what xHCI
revision the driver is running on.
Why are you only ringing the endpoint doorbell or stopping the endpoint
if the device is configured? It is possible that a driver or the USB
core has placed the device in the addressed mode with a SetAddress 0
control transfer, and we will want to talk to it when it comes out of
suspend. The USB 2.0 bus spec (section 7.1.7.6) says "Devices can go
into the Suspend state from any powered state."
It does not hurt to ring the doorbell unconditionally. If there is no
TDs on the ring, then nothing happens, other than the host controller
fetches a TD it doesn't own.
It also doesn't hurt to issue a stop endpoint command for a disabled
device. The stop endpoint command handler will ignore the status that
comes back for that command completion. In fact, the original code
won't do anything at all unless there are TDs on the cancellation list.
The USB core shouldn't suspend a device unless all URBs are completed
and the device has been idle for a while, so there should never be TDs
on the cancellation list.
> + ROOT_HUB_PORT(slot_ctx->dev_info2) == port) {
> + slot_id = i;
> + break;
> + }
> + }
> +
> + if (!slot_id)
> + return 0;
> +
> + if (stop) {
> + for (i = 0; i < 31; i++) {
> + ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, i);
> + if ((ep_ctx->ep_info & EP_STATE_MASK)
> + != EP_STATE_RUNNING)
> + continue;
> + ep = &xhci->devs[slot_id]->eps[i];
> + if (!(ep->ep_state & EP_HALT_PENDING)) {
> + ep->ep_state |= EP_HALT_PENDING;
> + ep->stop_cmds_pending++;
> + ep->stop_cmd_timer.expires = jiffies +
> + XHCI_STOP_EP_CMD_TIMEOUT * HZ;
> + add_timer(&ep->stop_cmd_timer);
> + xhci_queue_stop_endpoint(xhci, slot_id, i, 1);
> + xhci->devs[slot_id]->last_ep = i;
> + }
> + }
> + xhci_ring_cmd_db(xhci);
> + spin_unlock_irqrestore(&xhci->lock, *flags);
> + wait_for_completion(&xhci->devs[slot_id]->cmd_completion);
> + spin_lock_irqsave(&xhci->lock, *flags);
> + xhci->devs[slot_id]->last_ep = -1;
> + } else {
> + for (i = 0; i < 31; i++) {
> + ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, i);
> + if ((ep_ctx->ep_info & EP_STATE_MASK)
> + == EP_STATE_STOPPED)
> + ring_ep_doorbell(xhci, slot_id, i);
> + }
> + }
> + return 0;
> +}
>
> static void xhci_disable_port(struct xhci_hcd *xhci, u16 wIndex,
> u32 __iomem *addr, u32 port_status)
> @@ -162,6 +225,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;
> @@ -211,9 +278,21 @@ 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
> */
> + /* XDEV_U3 is Device Suspended, XDEV_RESUME is in resuming and
> + * the device may not directly transit in to (Running) state.
> + * So Report SUSPEND for simple.
> + */
> + if ((temp & PORT_PLS_MASK) == XDEV_U3 ||
> + (temp & PORT_PLS_MASK) == XDEV_RESUME)
> + status |= 1 << USB_PORT_FEAT_SUSPEND;
> + if ((temp & PORT_PLS_MASK) == XDEV_U0
> + && 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 +305,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 +319,31 @@ 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);
> + /* spec 4.15.1 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;
> + }
> + xhci_stop_endpoints(xhci, wIndex + 1, 1, &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 +377,43 @@ 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);
> + }
> + xhci_stop_endpoints(xhci, wIndex + 1, 0, &flags);
> + 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-mem.c b/drivers/usb/host/xhci-mem.c
> index 8045bc6..a48dee3 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -400,6 +400,7 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
>
> init_completion(&dev->cmd_completion);
> INIT_LIST_HEAD(&dev->cmd_list);
> + dev->last_ep = -1;
>
> /* Point to output device context in dcbaa. */
> xhci->dcbaa->dev_context_ptrs[slot_id] = dev->out_ctx->dma;
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 6ba841b..558c0e6 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1,4 +1,5 @@
> /*
> +int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
> * xHCI host controller driver
> *
> * Copyright (C) 2008 Intel Corp.
> @@ -292,7 +293,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)
> {
> @@ -524,6 +525,7 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci,
> struct list_head *entry;
> struct xhci_td *cur_td = 0;
> struct xhci_td *last_unlinked_td;
> + int last_ep;
>
> struct xhci_dequeue_state deq_state;
>
> @@ -532,6 +534,7 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci,
> ep_index = TRB_TO_EP_INDEX(trb->generic.field[3]);
> ep = &xhci->devs[slot_id]->eps[ep_index];
> ep_ring = ep->ring;
> + last_ep = xhci->devs[slot_id]->last_ep;
>
> if (list_empty(&ep->cancelled_td_list)) {
> xhci_stop_watchdog_timer_in_irq(xhci, ep);
> ring_ep_doorbell(xhci, slot_id, ep_index);
> return;
> }
Uh, did you think about the above case in the code? From the way I read
the modified code, the xHCI driver will issue several stop endpoint
commands with the suspend bit set, and then immediately restart them
with a doorbell ring, since the TD cancellation list is empty.
If that's true, your completion will never get called below. Since
you're using the generic completion in the device slot, you could be
getting a completion from some other command. It's probably better if
you allocate an xhci_command and stash it in the xhci_virt_device
structure.
So how can this patch work? Am I just reading the code wrong?
> @@ -602,6 +605,8 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci,
> return;
> } while (cur_td != last_unlinked_td);
>
> + if (last_ep == ep_index)
> + complete(&xhci->devs[slot_id]->cmd_completion);
> /* Return to the event handler with xhci->lock re-acquired */
> }
>
> @@ -2229,14 +2234,15 @@ int xhci_queue_evaluate_context(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
> }
>
> int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
> - unsigned int ep_index)
> + unsigned int ep_index, int sp)
Please call "sp" something else more descriptive, like
"suspending_device", and add a comment to xhci_queue_stop_endpoint() to
say what this variable is used for.
> {
> u32 trb_slot_id = SLOT_ID_FOR_TRB(slot_id);
> u32 trb_ep_index = EP_ID_FOR_TRB(ep_index);
> u32 type = TRB_TYPE(TRB_STOP_RING);
> + u32 trb_sp = SP_FOR_TRB(sp);
>
> return queue_command(xhci, 0, 0, 0,
> - trb_slot_id | trb_ep_index | type, false);
> + trb_slot_id | trb_ep_index | type | trb_sp, false);
> }
>
> /* Set Transfer Ring Dequeue Pointer command.
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index e5eb09b..b104177 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -269,6 +269,10 @@ struct xhci_op_regs {
> * A read gives the current link PM state of the port,
> * a write with Link State Write Strobe set sets the link state.
> */
> +#define PORT_PLS_MASK (0xf << 5)
> +#define XDEV_U0 (0x0 << 5)
> +#define XDEV_U3 (0x3 << 5)
> +#define XDEV_RESUME (0xf << 5)
> /* true: port has power (see HCC_PPC) */
> #define PORT_POWER (1 << 9)
> /* bits 10:13 indicate device speed:
> @@ -686,6 +690,10 @@ struct xhci_virt_device {
> /* Status of the last command issued for this device */
> u32 cmd_status;
> struct list_head cmd_list;
> + /* Inditcator of the last stop end point. Let low level function
> + * to judgement in what situation complete will be called.
> + */
> + int last_ep;
> };
>
>
> @@ -823,7 +831,7 @@ struct xhci_event_cmd {
> /* Stop Endpoint TRB - ep_index to endpoint ID for this TRB */
> #define TRB_TO_EP_INDEX(p) ((((p) & (0x1f << 16)) >> 16) - 1)
> #define EP_ID_FOR_TRB(p) ((((p) + 1) & 0x1f) << 16)
> -
> +#define SP_FOR_TRB(p) (((p) & 1) << 23)
Can you please change this to
#define SUSPEND_PORT_FOR_TRB(p) (((p) & 1) << 23)
I won't remember what "SP" is after 3 months, and I think neither will
you. :)
>
> /* Port Status Change Event TRB fields */
> /* Port ID - bits 31:24 */
> @@ -1112,6 +1120,9 @@ struct xhci_hcd {
> unsigned int quirks;
> #define XHCI_LINK_TRB_QUIRK (1 << 0)
> #define XHCI_RESET_EP_QUIRK (1 << 1)
> + unsigned long port_c_suspend; /* port suspend change*/
> + unsigned long suspended_ports; /* which ports are
> + suspended */
> };
>
> /* For testing purposes */
> @@ -1288,7 +1299,7 @@ int xhci_queue_slot_control(struct xhci_hcd *xhci, u32 trb_type, u32 slot_id);
> int xhci_queue_address_device(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
> u32 slot_id);
> int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
> - unsigned int ep_index);
> + unsigned int ep_index, int sp);
> int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags, struct urb *urb,
> int slot_id, unsigned int ep_index);
> int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags, struct urb *urb,
> @@ -1314,6 +1325,8 @@ void xhci_queue_config_ep_quirk(struct xhci_hcd *xhci,
> unsigned int slot_id, unsigned int ep_index,
> struct xhci_dequeue_state *deq_state);
> void xhci_stop_endpoint_command_watchdog(unsigned long arg);
> +void ring_ep_doorbell(struct xhci_hcd *xhci, unsigned int slot_id,
> + unsigned int ep_index);
>
> /* xHCI roothub code */
> int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
>
> --
> 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
--
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
