2014-01-30 14:02:52

by Mathias Nyman

Subject: [RFCv2 00/10] xhci: re-work command queue management

Only changes since v1 are fixing smatch warnings and errors.
patch 01/10
Check for null return from alloc_command, release lock in error path and
don't dereference possible null pointer in error path.

patch 04/10
release lock in xhci_stop_dev() error path.

This is the second attempt to re-work and solve the issues in xhci command
queue management that Sarah has descibed earlier:

Right now, the command management in the xHCI driver is rather ad-hock.
Different parts of the driver all submit commands, including interrupt
handling routines, functions called from the USB core (with or without the
bus bandwidth mutex held).
Some times they need to wait for the command to complete, and sometimes
they just issue the command and don't care about the result of the command.

The places that wait on a command all time the command for five seconds,
and then attempt to cancel the command.
Unfortunately, that means if several commands are issued at once, and one of
them times out, all the commands timeout, even though the host hasn't gotten
a chance to service them yet.

This is apparent with some devices that take a long time to respond to the
Set Address command during device enumeration (when the device is plugged in).
If a driver for a different device attempts to change alternate interface
settings at the same time (causing a Configure Endpoint command to be issued),
both commands timeout.

Instead of having each command timeout after five seconds, the driver should
wait indefinitely in an uninterruptible sleep on the command completion.
A global command queue manager should time whatever command is currently
running, and cancel that command after five seconds.

If the commands were in a list, like TDs currently are, it may be easier to keep
track of where the command ring dequeue pointer is, and avoid racing with events.
We may need to have parts of the driver that issue commands without waiting on
them still put the commands in the command list.

The Implementation:
-------------------

First step is to create a list of the commands submitted to the command queue.
To accomplish this each command is required to be submitted with a properly
filled command structure containing completion, status variable and a pointer to
the command TRB that will be used.

The first 7 patches are all about creating these command structures and
submitting them when we queue commands.
The command structures are allocated on the fly, the commands that are submitted
in interrupt context are allocated with GFP_ATOMIC.

Next, the global command queue is introduced. Commands are added to the queue
when trb's are queued, and remove when the commad completes.
Also switch to use the status variable and completion in the command struct.

A new timer handles command timeout, the timer is kicked every time when a
command finishes and there's a new command waiting in the queue, or when a new
command is submitted to an _empty_ command queue.
Timer is deleted when the the last command on the queue finishes (empty queue)

The old cancel_cmd_list is removed.
When the timer expires we simply tag the current command as "ABORTED" and start
the ring abortion process. Functions waiting for an aborted command to finish are
called after the command abortion is completed.

Mathias Nyman (10):
xhci: Use command structures when calling xhci_configure_endpoint
xhci: use a command structure internally in xhci_address_device()
xhci: use command structures for xhci_queue_slot_control()
xhci: use command structures for xhci_queue_stop_endpoint()
xhci: use command structure for xhci_queue_new_dequeue_state()
xhci: use command structures for xhci_queue_reset_ep()
xhci: Use command structured when queuing commands
xhci: Add a global command queue
xhci: Use completion and status in global command queue
xhci: rework command timeout and cancellation,

drivers/usb/host/xhci-hub.c | 43 ++--
drivers/usb/host/xhci-mem.c | 22 +-
drivers/usb/host/xhci-ring.c | 532 ++++++++++++++-----------------------------
drivers/usb/host/xhci.c | 266 ++++++++++++----------
drivers/usb/host/xhci.h | 43 ++--
5 files changed, 376 insertions(+), 530 deletions(-)

--
1.8.1.2

2014-01-30 14:02:47

by Mathias Nyman

Subject: [RFCv2 02/10] xhci: use a command structure internally in xhci_address_device()

Preparing for the global command queue by using command strucure
in xhci_address_device

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a40485e..5bf0f94 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3734,7 +3734,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
struct xhci_slot_ctx *slot_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
u64 temp_64;
- union xhci_trb *cmd_trb;
+ struct xhci_command *command;

if (!udev->slot_id) {
xhci_dbg_trace(xhci, trace_xhci_dbg_address,
@@ -3755,11 +3755,19 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
return -EINVAL;
}

+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ if (!command)
+ return -ENOMEM;
+
+ command->in_ctx = virt_dev->in_ctx;
+ command->completion = &xhci->addr_dev;
+
slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
ctrl_ctx = xhci_get_input_control_ctx(xhci, virt_dev->in_ctx);
if (!ctrl_ctx) {
xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
__func__);
+ kfree(command);
return -EINVAL;
}
/*
@@ -3781,20 +3789,22 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
slot_ctx->dev_info >> 27);

spin_lock_irqsave(&xhci->lock, flags);
- cmd_trb = xhci_find_next_enqueue(xhci->cmd_ring);
+ command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
ret = xhci_queue_address_device(xhci, virt_dev->in_ctx->dma,
udev->slot_id);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg_trace(xhci, trace_xhci_dbg_address,
"FIXME: allocate a command ring segment");
+ kfree(command);
return ret;
}
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);

/* ctrl tx can take up to 5 sec; XXX: need more time for xHC? */
- timeleft = wait_for_completion_interruptible_timeout(&xhci->addr_dev,
+ timeleft = wait_for_completion_interruptible_timeout(
+ command->completion,
XHCI_CMD_DEFAULT_TIMEOUT);
/* FIXME: From section 4.3.4: "Software shall be responsible for timing
* the SetAddress() "recovery interval" required by USB and aborting the
@@ -3804,7 +3814,8 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
xhci_warn(xhci, "%s while waiting for address device command\n",
timeleft == 0 ? "Timeout" : "Signal");
/* cancel the address device command */
- ret = xhci_cancel_cmd(xhci, NULL, cmd_trb);
+ ret = xhci_cancel_cmd(xhci, NULL, command->command_trb);
+ kfree(command);
if (ret < 0)
return ret;
return -ETIME;
@@ -3840,6 +3851,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
break;
}
if (ret) {
+ kfree(command);
return ret;
}
temp_64 = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
@@ -3874,7 +3886,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
xhci_dbg_trace(xhci, trace_xhci_dbg_address,
"Internal device address = %d",
le32_to_cpu(slot_ctx->dev_state) & DEV_ADDR_MASK);
-
+ kfree(command);
return 0;
}

--
1.8.1.2

2014-01-30 14:02:55

by Mathias Nyman

Subject: [RFCv2 05/10] xhci: use command structure for xhci_queue_new_dequeue_state()

Prepare for the global command ring by using command structures
internally in functions calling xhci_queue_new_dequeue_state()

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-ring.c | 3 +++
drivers/usb/host/xhci.c | 6 ++++++
2 files changed, 9 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index da83a844..df5b0f8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -855,11 +855,14 @@ remove_finished_td:

/* If necessary, queue a Set Transfer Ring Dequeue Pointer command */
if (deq_state.new_deq_ptr && deq_state.new_deq_seg) {
+ struct xhci_command *command;
+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
xhci_queue_new_dequeue_state(xhci,
slot_id, ep_index,
ep->stopped_td->urb->stream_id,
&deq_state);
xhci_ring_cmd_db(xhci);
+ kfree(command);
} else {
/* Otherwise ring the doorbell(s) to restart queued transfers */
ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d4684d0..0c3f0bc 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2869,10 +2869,16 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci,
* issue a configure endpoint command later.
*/
if (!(xhci->quirks & XHCI_RESET_EP_QUIRK)) {
+ struct xhci_command *command;
+ /* Can't sleep if we're called from cleanup_halted_endpoint() */
+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
+ if (!command)
+ return;
xhci_dbg_trace(xhci, trace_xhci_dbg_reset_ep,
"Queueing new dequeue state");
xhci_queue_new_dequeue_state(xhci, udev->slot_id,
ep_index, ep->stopped_stream, &deq_state);
+ kfree(command);
} else {
/* Better hope no one uses the input context between now and the
* reset endpoint completion!
--
1.8.1.2

2014-01-30 14:03:05

by Mathias Nyman

Subject: [RFCv2 08/10] xhci: Add a global command queue

Create a list to store command structures, add a strucure to it every time
a command is submitted, and remove it from the list once we get a
command completion event matching the command.

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-mem.c | 8 ++++++++
drivers/usb/host/xhci-ring.c | 13 ++++++++++++-
drivers/usb/host/xhci.h | 1 +
3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 49b8bd0..7f8e4c3 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1694,6 +1694,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
struct xhci_cd *cur_cd, *next_cd;
+ struct xhci_command *cur_cmd, *next_cmd;
int size;
int i, j, num_ports;

@@ -1722,6 +1723,12 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
kfree(cur_cd);
}

+ list_for_each_entry_safe(cur_cmd, next_cmd,
+ &xhci->cmd_list, cmd_list) {
+ list_del(&cur_cmd->cmd_list);
+ kfree(cur_cmd);
+ }
+
for (i = 1; i < MAX_HC_SLOTS; ++i)
xhci_free_virt_device(xhci, i);

@@ -2223,6 +2230,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
int i;

INIT_LIST_HEAD(&xhci->cancel_cmd_list);
+ INIT_LIST_HEAD(&xhci->cmd_list);

page_size = xhci_readl(xhci, &xhci->op_regs->page_size);
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6f6018c..5ccf642 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1502,6 +1502,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
dma_addr_t cmd_dequeue_dma;
u32 cmd_comp_code;
union xhci_trb *cmd_trb;
+ struct xhci_command *cmd;
u32 cmd_type;

cmd_dma = le64_to_cpu(event->cmd_trb);
@@ -1519,6 +1520,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
return;
}

+ cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);
+
+ if (cmd->command_trb != xhci->cmd_ring->dequeue) {
+ xhci_err(xhci,
+ "Command completion event does not match command\n");
+ return;
+ }
trace_xhci_cmd_completion(cmd_trb, (struct xhci_generic_trb *) event);

cmd_comp_code = GET_COMP_CODE(le32_to_cpu(event->status));
@@ -1588,6 +1596,9 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
xhci->error_bitmask |= 1 << 6;
break;
}
+
+ list_del(&cmd->cmd_list);
+
inc_deq(xhci, xhci->cmd_ring);
}

@@ -3989,7 +4000,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
return ret;
}
cmd->command_trb = xhci->cmd_ring->enqueue;
-
+ list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
field4 | xhci->cmd_ring->cycle_state);
return 0;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index d02b73d..71aed35 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1475,6 +1475,7 @@ struct xhci_hcd {
#define CMD_RING_STATE_ABORTED (1 << 1)
#define CMD_RING_STATE_STOPPED (1 << 2)
struct list_head cancel_cmd_list;
+ struct list_head cmd_list;
unsigned int cmd_ring_reserved_trbs;
struct xhci_ring *event_ring;
struct xhci_erst erst;
--
1.8.1.2

2014-01-30 14:03:03

by Mathias Nyman

Subject: [RFCv2 07/10] xhci: Use command structured when queuing commands

Require each queued command to have a command structure.
We store the command trb pointer in the structure when queuing it,
making the find_next_enqueue() function obsolete.

Don't free the command strucures right away after sending the commands,
We will free the commands when we receive a "command handled" event
in a later patch.

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-hub.c | 8 ++--
drivers/usb/host/xhci-ring.c | 94 ++++++++++++++++++++++----------------------
drivers/usb/host/xhci.c | 47 +++++++++++-----------
drivers/usb/host/xhci.h | 31 ++++++++-------
4 files changed, 91 insertions(+), 89 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 68bc1c6..4342ec3 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -293,14 +293,14 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_free_command(xhci, cmd);
return -ENOMEM;
+
}
- xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
- kfree(command);
+ xhci_queue_stop_endpoint(xhci, command, slot_id, i,
+ suspend);
}
}
- cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
- xhci_queue_stop_endpoint(xhci, slot_id, 0, suspend);
+ xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index bf50d28..6f6018c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -123,16 +123,6 @@ static int enqueue_is_link_trb(struct xhci_ring *ring)
return TRB_TYPE_LINK_LE32(link->control);
}

-union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring)
-{
- /* Enqueue pointer can be left pointing to the link TRB,
- * we must handle that
- */
- if (TRB_TYPE_LINK_LE32(ring->enqueue->link.control))
- return ring->enq_seg->next->trbs;
- return ring->enqueue;
-}
-
/* Updates trb to point to the next TRB in the ring, and updates seg if the next
* TRB is in a new segment. This does not skip over link TRBs, and it does not
* effect the ring dequeue or enqueue pointers.
@@ -680,12 +670,14 @@ static void td_to_noop(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
}
}

-static int queue_set_tr_deq(struct xhci_hcd *xhci, int slot_id,
+static int queue_set_tr_deq(struct xhci_hcd *xhci,
+ struct xhci_command *cmd, int slot_id,
unsigned int ep_index, unsigned int stream_id,
struct xhci_segment *deq_seg,
union xhci_trb *deq_ptr, u32 cycle_state);

void xhci_queue_new_dequeue_state(struct xhci_hcd *xhci,
+ struct xhci_command *cmd,
unsigned int slot_id, unsigned int ep_index,
unsigned int stream_id,
struct xhci_dequeue_state *deq_state)
@@ -700,7 +692,7 @@ void xhci_queue_new_dequeue_state(struct xhci_hcd *xhci,
deq_state->new_deq_ptr,
(unsigned long long)xhci_trb_virt_to_dma(deq_state->new_deq_seg, deq_state->new_deq_ptr),
deq_state->new_cycle_state);
- queue_set_tr_deq(xhci, slot_id, ep_index, stream_id,
+ queue_set_tr_deq(xhci, cmd, slot_id, ep_index, stream_id,
deq_state->new_deq_seg,
deq_state->new_deq_ptr,
(u32) deq_state->new_cycle_state);
@@ -857,12 +849,11 @@ remove_finished_td:
if (deq_state.new_deq_ptr && deq_state.new_deq_seg) {
struct xhci_command *command;
command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
- xhci_queue_new_dequeue_state(xhci,
+ xhci_queue_new_dequeue_state(xhci, command,
slot_id, ep_index,
ep->stopped_td->urb->stream_id,
&deq_state);
xhci_ring_cmd_db(xhci);
- kfree(command);
} else {
/* Otherwise ring the doorbell(s) to restart queued transfers */
ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
@@ -1187,11 +1178,10 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
"Queueing configure endpoint command");
- xhci_queue_configure_endpoint(xhci,
+ xhci_queue_configure_endpoint(xhci, command,
xhci->devs[slot_id]->in_ctx->dma, slot_id,
false);
xhci_ring_cmd_db(xhci);
- kfree(command);
} else {
/* Clear our internal halted state and restart the ring(s) */
xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_HALTED;
@@ -1922,7 +1912,7 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
ep->stopped_trb = event_trb;
ep->stopped_stream = stream_id;

- xhci_queue_reset_ep(xhci, slot_id, ep_index);
+ xhci_queue_reset_ep(xhci, command, slot_id, ep_index);
xhci_cleanup_stalled_ring(xhci, td->urb->dev, ep_index);

ep->stopped_td = NULL;
@@ -1930,7 +1920,6 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
ep->stopped_stream = 0;

xhci_ring_cmd_db(xhci);
- kfree(command);
}

/* Check if an error has halted the endpoint ring. The class driver will
@@ -2638,7 +2627,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* successful event after a short transfer.
* Ignore it.
*/
- if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
+ if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
ep_ring->last_td_was_short) {
ep_ring->last_td_was_short = false;
ret = 0;
@@ -3980,8 +3969,9 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
* Don't decrement xhci->cmd_ring_reserved_trbs after we've queued the TRB
* because the command event handler may want to resubmit a failed command.
*/
-static int queue_command(struct xhci_hcd *xhci, u32 field1, u32 field2,
- u32 field3, u32 field4, bool command_must_succeed)
+static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ u32 field1, u32 field2,
+ u32 field3, u32 field4, bool command_must_succeed)
{
int reserved_trbs = xhci->cmd_ring_reserved_trbs;
int ret;
@@ -3998,57 +3988,64 @@ static int queue_command(struct xhci_hcd *xhci, u32 field1, u32 field2,
"unfailable commands failed.\n");
return ret;
}
+ cmd->command_trb = xhci->cmd_ring->enqueue;
+
queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
field4 | xhci->cmd_ring->cycle_state);
return 0;
}

/* Queue a slot enable or disable request on the command ring */
-int xhci_queue_slot_control(struct xhci_hcd *xhci, u32 trb_type, u32 slot_id)
+int xhci_queue_slot_control(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ u32 trb_type, u32 slot_id)
{
- return queue_command(xhci, 0, 0, 0,
+ return queue_command(xhci, cmd, 0, 0, 0,
TRB_TYPE(trb_type) | SLOT_ID_FOR_TRB(slot_id), false);
}

/* Queue an address device command TRB */
-int xhci_queue_address_device(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
- u32 slot_id)
+int xhci_queue_address_device(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ dma_addr_t in_ctx_ptr, u32 slot_id)
{
- return queue_command(xhci, lower_32_bits(in_ctx_ptr),
+ return queue_command(xhci, cmd, lower_32_bits(in_ctx_ptr),
upper_32_bits(in_ctx_ptr), 0,
TRB_TYPE(TRB_ADDR_DEV) | SLOT_ID_FOR_TRB(slot_id),
false);
}

-int xhci_queue_vendor_command(struct xhci_hcd *xhci,
+int xhci_queue_vendor_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
u32 field1, u32 field2, u32 field3, u32 field4)
{
- return queue_command(xhci, field1, field2, field3, field4, false);
+ return queue_command(xhci, cmd, field1, field2, field3, field4, false);
}

/* Queue a reset device command TRB */
-int xhci_queue_reset_device(struct xhci_hcd *xhci, u32 slot_id)
+int xhci_queue_reset_device(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ u32 slot_id)
{
- return queue_command(xhci, 0, 0, 0,
+ return queue_command(xhci, cmd, 0, 0, 0,
TRB_TYPE(TRB_RESET_DEV) | SLOT_ID_FOR_TRB(slot_id),
false);
}

/* Queue a configure endpoint command TRB */
-int xhci_queue_configure_endpoint(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
- u32 slot_id, bool command_must_succeed)
+int xhci_queue_configure_endpoint(struct xhci_hcd *xhci,
+ struct xhci_command *cmd,
+ dma_addr_t in_ctx_ptr,
+ u32 slot_id, bool command_must_succeed)
{
- return queue_command(xhci, lower_32_bits(in_ctx_ptr),
+ return queue_command(xhci, cmd, lower_32_bits(in_ctx_ptr),
upper_32_bits(in_ctx_ptr), 0,
TRB_TYPE(TRB_CONFIG_EP) | SLOT_ID_FOR_TRB(slot_id),
command_must_succeed);
}

/* Queue an evaluate context command TRB */
-int xhci_queue_evaluate_context(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
- u32 slot_id, bool command_must_succeed)
+int xhci_queue_evaluate_context(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ dma_addr_t in_ctx_ptr,
+ u32 slot_id, bool command_must_succeed)
{
- return queue_command(xhci, lower_32_bits(in_ctx_ptr),
+ return queue_command(xhci, cmd, lower_32_bits(in_ctx_ptr),
upper_32_bits(in_ctx_ptr), 0,
TRB_TYPE(TRB_EVAL_CONTEXT) | SLOT_ID_FOR_TRB(slot_id),
command_must_succeed);
@@ -4058,25 +4055,26 @@ int xhci_queue_evaluate_context(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
* Suspend is set to indicate "Stop Endpoint Command" is being issued to stop
* activity on an endpoint that is about to be suspended.
*/
-int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index, int suspend)
+int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ int slot_id, unsigned int ep_index, int suspend)
{
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_suspend = SUSPEND_PORT_FOR_TRB(suspend);

- return queue_command(xhci, 0, 0, 0,
+ return queue_command(xhci, cmd, 0, 0, 0,
trb_slot_id | trb_ep_index | type | trb_suspend, false);
}

/* Set Transfer Ring Dequeue Pointer command.
* This should not be used for endpoints that have streams enabled.
*/
-static int queue_set_tr_deq(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index, unsigned int stream_id,
- struct xhci_segment *deq_seg,
- union xhci_trb *deq_ptr, u32 cycle_state)
+static int queue_set_tr_deq(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ int slot_id,
+ unsigned int ep_index, unsigned int stream_id,
+ struct xhci_segment *deq_seg,
+ union xhci_trb *deq_ptr, u32 cycle_state)
{
dma_addr_t addr;
u32 trb_slot_id = SLOT_ID_FOR_TRB(slot_id);
@@ -4100,18 +4098,18 @@ static int queue_set_tr_deq(struct xhci_hcd *xhci, int slot_id,
}
ep->queued_deq_seg = deq_seg;
ep->queued_deq_ptr = deq_ptr;
- return queue_command(xhci, lower_32_bits(addr) | cycle_state,
+ return queue_command(xhci, cmd, lower_32_bits(addr) | cycle_state,
upper_32_bits(addr), trb_stream_id,
trb_slot_id | trb_ep_index | type, false);
}

-int xhci_queue_reset_ep(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index)
+int xhci_queue_reset_ep(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ int slot_id, unsigned int ep_index)
{
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_RESET_EP);

- return queue_command(xhci, 0, 0, 0, trb_slot_id | trb_ep_index | type,
- false);
+ return queue_command(xhci, cmd, 0, 0, 0,
+ trb_slot_id | trb_ep_index | type, false);
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 37a1a7e..393d658 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -631,10 +631,14 @@ int xhci_run(struct usb_hcd *hcd)
&xhci->ir_set->irq_pending);
xhci_print_ir_set(xhci, 0);

- if (xhci->quirks & XHCI_NEC_HOST)
- xhci_queue_vendor_command(xhci, 0, 0, 0,
+ if (xhci->quirks & XHCI_NEC_HOST) {
+ struct xhci_command *command;
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ if (!command)
+ return -ENOMEM;
+ xhci_queue_vendor_command(xhci, command, 0, 0, 0,
TRB_TYPE(TRB_NEC_GET_FW));
-
+ }
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"Finished xhci_run for USB2 roothub");
return 0;
@@ -1542,9 +1546,9 @@ 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, 0);
+ xhci_queue_stop_endpoint(xhci, command, urb->dev->slot_id,
+ ep_index, 0);
xhci_ring_cmd_db(xhci);
- kfree(command);
}
done:
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -2614,14 +2618,15 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
return -ENOMEM;
}

- command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
list_add_tail(&command->cmd_list, &virt_dev->cmd_list);

if (!ctx_change)
- ret = xhci_queue_configure_endpoint(xhci, command->in_ctx->dma,
+ ret = xhci_queue_configure_endpoint(xhci, command,
+ command->in_ctx->dma,
udev->slot_id, must_succeed);
else
- ret = xhci_queue_evaluate_context(xhci, command->in_ctx->dma,
+ ret = xhci_queue_evaluate_context(xhci, command,
+ command->in_ctx->dma,
udev->slot_id, must_succeed);
if (ret < 0) {
list_del(&command->cmd_list);
@@ -2876,9 +2881,8 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci,
return;
xhci_dbg_trace(xhci, trace_xhci_dbg_reset_ep,
"Queueing new dequeue state");
- xhci_queue_new_dequeue_state(xhci, udev->slot_id,
+ xhci_queue_new_dequeue_state(xhci, command, udev->slot_id,
ep_index, ep->stopped_stream, &deq_state);
- kfree(command);
} else {
/* Better hope no one uses the input context between now and the
* reset endpoint completion!
@@ -2938,7 +2942,7 @@ void xhci_endpoint_reset(struct usb_hcd *hcd,
xhci_dbg_trace(xhci, trace_xhci_dbg_reset_ep,
"Queueing reset endpoint command");
spin_lock_irqsave(&xhci->lock, flags);
- ret = xhci_queue_reset_ep(xhci, udev->slot_id, ep_index);
+ ret = xhci_queue_reset_ep(xhci, command, udev->slot_id, ep_index);
/*
* Can't change the ring dequeue pointer until it's transitioned to the
* stopped state, which is only upon a successful reset endpoint
@@ -2953,7 +2957,6 @@ void xhci_endpoint_reset(struct usb_hcd *hcd,
virt_ep->stopped_trb = NULL;
virt_ep->stopped_stream = 0;
spin_unlock_irqrestore(&xhci->lock, flags);
- kfree(command);

if (ret)
xhci_warn(xhci, "FIXME allocate a new ring segment\n");
@@ -3465,10 +3468,9 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)

/* Attempt to submit the Reset Device command to the command ring */
spin_lock_irqsave(&xhci->lock, flags);
- reset_device_cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);

list_add_tail(&reset_device_cmd->cmd_list, &virt_dev->cmd_list);
- ret = xhci_queue_reset_device(xhci, slot_id);
+ ret = xhci_queue_reset_device(xhci, reset_device_cmd, slot_id);
if (ret) {
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
list_del(&reset_device_cmd->cmd_list);
@@ -3623,15 +3625,14 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
return;
}

- if (xhci_queue_slot_control(xhci, TRB_DISABLE_SLOT, udev->slot_id)) {
+ if (xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
+ udev->slot_id)) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
- kfree(command);
return;
}
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
- kfree(command);

/*
* Event command completion handler will free any data structures
@@ -3679,9 +3680,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
return 0;

spin_lock_irqsave(&xhci->lock, flags);
- command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
command->completion = &xhci->addr_dev;
- ret = xhci_queue_slot_control(xhci, TRB_ENABLE_SLOT, 0);
+ ret = xhci_queue_slot_control(xhci, command, TRB_ENABLE_SLOT, 0);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
@@ -3750,10 +3750,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
disable_slot:
/* Disable slot, if we can do it without mem alloc */
spin_lock_irqsave(&xhci->lock, flags);
- if (!xhci_queue_slot_control(xhci, TRB_DISABLE_SLOT, udev->slot_id))
+ command->completion = NULL;
+ command->status = 0;
+ if (!xhci_queue_slot_control(xhci, command, TRB_DISABLE_SLOT,
+ udev->slot_id))
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
- kfree(command);
return 0;
}

@@ -3828,8 +3830,7 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
slot_ctx->dev_info >> 27);

spin_lock_irqsave(&xhci->lock, flags);
- command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
- ret = xhci_queue_address_device(xhci, virt_dev->in_ctx->dma,
+ ret = xhci_queue_address_device(xhci, command, virt_dev->in_ctx->dma,
udev->slot_id);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 03c74b7..d02b73d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1811,13 +1811,14 @@ struct xhci_segment *trb_in_td(struct xhci_segment *start_seg,
dma_addr_t suspect_dma);
int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code);
void xhci_ring_cmd_db(struct xhci_hcd *xhci);
-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_vendor_command(struct xhci_hcd *xhci,
+int xhci_queue_slot_control(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ u32 trb_type, u32 slot_id);
+int xhci_queue_address_device(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ dma_addr_t in_ctx_ptr, u32 slot_id);
+int xhci_queue_vendor_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
u32 field1, u32 field2, u32 field3, u32 field4);
-int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index, int suspend);
+int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ int slot_id, unsigned int ep_index, int suspend);
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,
@@ -1826,18 +1827,21 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t mem_flags, struct urb *urb,
int slot_id, unsigned int ep_index);
int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index);
-int xhci_queue_configure_endpoint(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
- u32 slot_id, bool command_must_succeed);
-int xhci_queue_evaluate_context(struct xhci_hcd *xhci, dma_addr_t in_ctx_ptr,
- u32 slot_id, bool command_must_succeed);
-int xhci_queue_reset_ep(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index);
-int xhci_queue_reset_device(struct xhci_hcd *xhci, u32 slot_id);
+int xhci_queue_configure_endpoint(struct xhci_hcd *xhci,
+ struct xhci_command *cmd, dma_addr_t in_ctx_ptr, u32 slot_id,
+ bool command_must_succeed);
+int xhci_queue_evaluate_context(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ dma_addr_t in_ctx_ptr, u32 slot_id, bool command_must_succeed);
+int xhci_queue_reset_ep(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ int slot_id, unsigned int ep_index);
+int xhci_queue_reset_device(struct xhci_hcd *xhci, struct xhci_command *cmd,
+ u32 slot_id);
void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
unsigned int slot_id, unsigned int ep_index,
unsigned int stream_id, struct xhci_td *cur_td,
struct xhci_dequeue_state *state);
void xhci_queue_new_dequeue_state(struct xhci_hcd *xhci,
+ struct xhci_command *cmd,
unsigned int slot_id, unsigned int ep_index,
unsigned int stream_id,
struct xhci_dequeue_state *deq_state);
@@ -1851,7 +1855,6 @@ int xhci_cancel_cmd(struct xhci_hcd *xhci, struct xhci_command *command,
union xhci_trb *cmd_trb);
void xhci_ring_ep_doorbell(struct xhci_hcd *xhci, unsigned int slot_id,
unsigned int ep_index, unsigned int stream_id);
-union xhci_trb *xhci_find_next_enqueue(struct xhci_ring *ring);

/* xHCI roothub code */
void xhci_set_link_state(struct xhci_hcd *xhci, __le32 __iomem **port_array,
--
1.8.1.2

2014-01-30 14:03:01

by Mathias Nyman

Subject: [RFCv2 09/10] xhci: Use completion and status in global command queue

Remove the per-device command list and handle_cmd_in_cmd_wait_list()
and use the completion and status variables found in the
command structure in the global command list.

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-hub.c | 11 ------
drivers/usb/host/xhci-mem.c | 1 -
drivers/usb/host/xhci-ring.c | 79 ++++++++------------------------------------
drivers/usb/host/xhci.c | 20 ++---------
drivers/usb/host/xhci.h | 3 --
5 files changed, 17 insertions(+), 97 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 4342ec3..1079cc6 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -299,7 +299,6 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
suspend);
}
}
- list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -311,18 +310,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for stop endpoint 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;
}
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 7f8e4c3..80b9c11 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -919,7 +919,6 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
dev->num_rings_cached = 0;

init_completion(&dev->cmd_completion);
- INIT_LIST_HEAD(&dev->cmd_list);
dev->udev = udev;

/* Point to output device context in dcbaa. */
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 5ccf642..6b61787 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -69,10 +69,6 @@
#include "xhci.h"
#include "xhci-trace.h"

-static int handle_cmd_in_cmd_wait_list(struct xhci_hcd *xhci,
- struct xhci_virt_device *virt_dev,
- struct xhci_event_cmd *event);
-
/*
* Returns zero if the TRB isn't in this segment, otherwise it returns the DMA
* address of the TRB.
@@ -761,7 +757,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
union xhci_trb *trb, struct xhci_event_cmd *event)
{
unsigned int ep_index;
- struct xhci_virt_device *virt_dev;
struct xhci_ring *ep_ring;
struct xhci_virt_ep *ep;
struct list_head *entry;
@@ -771,11 +766,7 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
struct xhci_dequeue_state deq_state;

if (unlikely(TRB_TO_SUSPEND_PORT(le32_to_cpu(trb->generic.field[3])))) {
- virt_dev = xhci->devs[slot_id];
- if (virt_dev)
- handle_cmd_in_cmd_wait_list(xhci, virt_dev,
- event);
- else
+ if (!xhci->devs[slot_id])
xhci_warn(xhci, "Stop endpoint command "
"completion for disabled slot %u\n",
slot_id);
@@ -1203,29 +1194,6 @@ static void xhci_complete_cmd_in_cmd_wait_list(struct xhci_hcd *xhci,
}


-/* Check to see if a command in the device's command queue matches this one.
- * Signal the completion or free the command, and return 1. Return 0 if the
- * completed command isn't at the head of the command list.
- */
-static int handle_cmd_in_cmd_wait_list(struct xhci_hcd *xhci,
- struct xhci_virt_device *virt_dev,
- struct xhci_event_cmd *event)
-{
- struct xhci_command *command;
-
- if (list_empty(&virt_dev->cmd_list))
- return 0;
-
- command = list_entry(virt_dev->cmd_list.next,
- struct xhci_command, cmd_list);
- if (xhci->cmd_ring->dequeue != command->command_trb)
- return 0;
-
- xhci_complete_cmd_in_cmd_wait_list(xhci, command,
- GET_COMP_CODE(le32_to_cpu(event->status)));
- return 1;
-}
-
/*
* Finding the command trb need to be cancelled and modifying it to
* NO OP command. And if the command is in device's command wait
@@ -1377,7 +1345,6 @@ static void xhci_handle_cmd_enable_slot(struct xhci_hcd *xhci, int slot_id,
xhci->slot_id = slot_id;
else
xhci->slot_id = 0;
- complete(&xhci->addr_dev);
}

static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int slot_id)
@@ -1402,9 +1369,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
unsigned int ep_state;
u32 add_flags, drop_flags;

- virt_dev = xhci->devs[slot_id];
- if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event))
- return;
/*
* Configure endpoint commands can come from the USB core
* configuration or alt setting changes, or because the HW
@@ -1413,6 +1377,7 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
* If the command was for a halted endpoint, the xHCI driver
* is not waiting on the configure endpoint command.
*/
+ virt_dev = xhci->devs[slot_id];
ctrl_ctx = xhci_get_input_control_ctx(xhci, virt_dev->in_ctx);
if (!ctrl_ctx) {
xhci_warn(xhci, "Could not get input context, bad type.\n");
@@ -1448,35 +1413,11 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
return;
}

-static void xhci_handle_cmd_eval_ctx(struct xhci_hcd *xhci, int slot_id,
- struct xhci_event_cmd *event, u32 cmd_comp_code)
-{
- struct xhci_virt_device *virt_dev;
-
- virt_dev = xhci->devs[slot_id];
- if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event))
- return;
- virt_dev->cmd_status = cmd_comp_code;
- complete(&virt_dev->cmd_completion);
-}
-
-static void xhci_handle_cmd_addr_dev(struct xhci_hcd *xhci, int slot_id,
- u32 cmd_comp_code)
-{
- xhci->devs[slot_id]->cmd_status = cmd_comp_code;
- complete(&xhci->addr_dev);
-}
-
static void xhci_handle_cmd_reset_dev(struct xhci_hcd *xhci, int slot_id,
struct xhci_event_cmd *event)
{
- struct xhci_virt_device *virt_dev;
-
xhci_dbg(xhci, "Completed reset device command.\n");
- virt_dev = xhci->devs[slot_id];
- if (virt_dev)
- handle_cmd_in_cmd_wait_list(xhci, virt_dev, event);
- else
+ if (!xhci->devs[slot_id])
xhci_warn(xhci, "Reset device command completion "
"for disabled slot %u\n", slot_id);
}
@@ -1558,13 +1499,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
xhci_handle_cmd_disable_slot(xhci, slot_id);
break;
case TRB_CONFIG_EP:
- xhci_handle_cmd_config_ep(xhci, slot_id, event, cmd_comp_code);
+ if (!cmd->completion)
+ xhci_handle_cmd_config_ep(xhci, slot_id, event,
+ cmd_comp_code);
break;
case TRB_EVAL_CONTEXT:
- xhci_handle_cmd_eval_ctx(xhci, slot_id, event, cmd_comp_code);
break;
case TRB_ADDR_DEV:
- xhci_handle_cmd_addr_dev(xhci, slot_id, cmd_comp_code);
break;
case TRB_STOP_RING:
WARN_ON(slot_id != TRB_TO_SLOT_ID(
@@ -1599,6 +1540,14 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,

list_del(&cmd->cmd_list);

+ if (cmd->completion) {
+ /* the caller waiting for completion should free the command */
+ cmd->status = cmd_comp_code;
+ complete(cmd->completion);
+ } else {
+ kfree(cmd);
+ }
+
inc_deq(xhci, xhci->cmd_ring);
}

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 393d658..e02c80e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2618,8 +2618,6 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
return -ENOMEM;
}

- list_add_tail(&command->cmd_list, &virt_dev->cmd_list);
-
if (!ctx_change)
ret = xhci_queue_configure_endpoint(xhci, command,
command->in_ctx->dma,
@@ -2629,7 +2627,6 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
command->in_ctx->dma,
udev->slot_id, must_succeed);
if (ret < 0) {
- list_del(&command->cmd_list);
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK))
xhci_free_host_resources(xhci, ctrl_ctx);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -3469,11 +3466,9 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
/* Attempt to submit the Reset Device command to the command ring */
spin_lock_irqsave(&xhci->lock, flags);

- list_add_tail(&reset_device_cmd->cmd_list, &virt_dev->cmd_list);
ret = xhci_queue_reset_device(xhci, reset_device_cmd, slot_id);
if (ret) {
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
- list_del(&reset_device_cmd->cmd_list);
spin_unlock_irqrestore(&xhci->lock, flags);
goto command_cleanup;
}
@@ -3487,13 +3482,6 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
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 (reset_device_cmd->cmd_list.next != LIST_POISON1)
- list_del(&reset_device_cmd->cmd_list);
- spin_unlock_irqrestore(&xhci->lock, flags);
ret = -ETIME;
goto command_cleanup;
}
@@ -3700,7 +3688,6 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
timeleft == 0 ? "Timeout" : "Signal");
/* cancel the enable slot request */
ret = xhci_cancel_cmd(xhci, NULL, command->command_trb);
- kfree(command);
return ret;
}

@@ -3855,13 +3842,12 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
timeleft == 0 ? "Timeout" : "Signal");
/* cancel the address device command */
ret = xhci_cancel_cmd(xhci, NULL, command->command_trb);
- kfree(command);
if (ret < 0)
return ret;
return -ETIME;
}

- switch (virt_dev->cmd_status) {
+ switch (command->status) {
case COMP_CTX_STATE:
case COMP_EBADSLT:
xhci_err(xhci, "Setup ERROR: address device command for slot %d.\n",
@@ -3882,8 +3868,8 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
"Successful Address Device command");
break;
default:
- xhci_err(xhci, "ERROR: unexpected command completion "
- "code 0x%x.\n", virt_dev->cmd_status);
+ xhci_err(xhci, "ERROR: unexpected command completion code 0x%x\n.",
+ command->status);
xhci_dbg(xhci, "Slot ID %d Output Context:\n", udev->slot_id);
xhci_dbg_ctx(xhci, virt_dev->out_ctx, 2);
trace_xhci_address_ctx(xhci, virt_dev->out_ctx, 1);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 71aed35..afff1da 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -938,9 +938,6 @@ struct xhci_virt_device {
#define XHCI_MAX_RINGS_CACHED 31
struct xhci_virt_ep eps[31];
struct completion cmd_completion;
- /* Status of the last command issued for this device */
- u32 cmd_status;
- struct list_head cmd_list;
u8 fake_port;
u8 real_port;
struct xhci_interval_bw_table *bw_table;
--
1.8.1.2

2014-01-30 14:04:20

by Mathias Nyman

Subject: [RFCv2 06/10] xhci: use command structures for xhci_queue_reset_ep()

Prepare for the global command ring by using command structures
internally in functions calling xhci_queue_reset_ep()

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-ring.c | 5 +++++
drivers/usb/host/xhci.c | 6 ++++++
2 files changed, 11 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index df5b0f8..bf50d28 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1913,6 +1913,10 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
struct xhci_td *td, union xhci_trb *event_trb)
{
struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
+ struct xhci_command *command;
+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
+ if (!command)
+ return;
ep->ep_state |= EP_HALTED;
ep->stopped_td = td;
ep->stopped_trb = event_trb;
@@ -1926,6 +1930,7 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
ep->stopped_stream = 0;

xhci_ring_cmd_db(xhci);
+ kfree(command);
}

/* Check if an error has halted the endpoint ring. The class driver will
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0c3f0bc..37a1a7e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2908,6 +2908,7 @@ void xhci_endpoint_reset(struct usb_hcd *hcd,
unsigned long flags;
int ret;
struct xhci_virt_ep *virt_ep;
+ struct xhci_command *command;

xhci = hcd_to_xhci(hcd);
udev = (struct usb_device *) ep->hcpriv;
@@ -2930,6 +2931,10 @@ void xhci_endpoint_reset(struct usb_hcd *hcd,
return;
}

+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
+ if (!command)
+ return;
+
xhci_dbg_trace(xhci, trace_xhci_dbg_reset_ep,
"Queueing reset endpoint command");
spin_lock_irqsave(&xhci->lock, flags);
@@ -2948,6 +2953,7 @@ void xhci_endpoint_reset(struct usb_hcd *hcd,
virt_ep->stopped_trb = NULL;
virt_ep->stopped_stream = 0;
spin_unlock_irqrestore(&xhci->lock, flags);
+ kfree(command);

if (ret)
xhci_warn(xhci, "FIXME allocate a new ring segment\n");
--
1.8.1.2

2014-01-30 14:04:38

by Mathias Nyman

Subject: [RFCv2 10/10] xhci: rework command timeout and cancellation,

Use one timer to control command timeout.

start/kick the timer every time a command is completed and a
new command is waiting, or a new command is added to a empty list.

If the timer runs out, then tag the current command as "aborted", and
start the xhci command abortion process.

Previously each function that submitted a command had its own timer.
If that command timed out, a new command structure for
the command was created and it was put on a cancel_cmd_list list,
then a pci write to abort the command ring was issued.

when the ring was aborted, it checked if the current command
was the one to be cancelled, later when the ring was stopped the
driver got ownership of the TRBs in the command ring,
compared then to the TRBs in the cancel_cmd_list,
and turned them into No-ops.

Now, instead, at timeout we tag the status of the command in the
command queue to be aborted, and start the ring abortion.
Ring abortion stops the command ring and gives control of the commands to us.
All the aborted commands are now turned into No-ops.

This allows us to remove the entire cancel_cmd_list code.

The functions waiting for a command to finish no longer have their own timeouts.
They will wait either until the command completes normally,
or until the whole command abortion is done.

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-hub.c | 11 +-
drivers/usb/host/xhci-mem.c | 15 +-
drivers/usb/host/xhci-ring.c | 336 +++++++++++++------------------------------
drivers/usb/host/xhci.c | 79 ++++------
drivers/usb/host/xhci.h | 8 +-
5 files changed, 139 insertions(+), 310 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 1079cc6..5df5fcf 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -271,7 +271,6 @@ 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;

@@ -304,12 +303,10 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
spin_unlock_irqrestore(&xhci->lock, flags);

/* Wait for last stop endpoint command to finish */
- timeleft = wait_for_completion_interruptible_timeout(
- cmd->completion,
- XHCI_CMD_DEFAULT_TIMEOUT);
- if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for stop endpoint command\n",
- timeleft == 0 ? "Timeout" : "Signal");
+ wait_for_completion(cmd->completion);
+
+ if (cmd->status == COMP_CMD_ABORT || cmd->status == COMP_CMD_STOP) {
+ xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
ret = -ETIME;
}
xhci_free_command(xhci, cmd);
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 80b9c11..d27178b 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1692,7 +1692,6 @@ void xhci_free_command(struct xhci_hcd *xhci,
void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
- struct xhci_cd *cur_cd, *next_cd;
struct xhci_command *cur_cmd, *next_cmd;
int size;
int i, j, num_ports;
@@ -1712,15 +1711,13 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
if (xhci->lpm_command)
xhci_free_command(xhci, xhci->lpm_command);
xhci->cmd_ring_reserved_trbs = 0;
+
+ del_timer_sync(&xhci->cmd_timer);
+
if (xhci->cmd_ring)
xhci_ring_free(xhci, xhci->cmd_ring);
xhci->cmd_ring = NULL;
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed command ring");
- list_for_each_entry_safe(cur_cd, next_cd,
- &xhci->cancel_cmd_list, cancel_cmd_list) {
- list_del(&cur_cd->cancel_cmd_list);
- kfree(cur_cd);
- }

list_for_each_entry_safe(cur_cmd, next_cmd,
&xhci->cmd_list, cmd_list) {
@@ -2228,7 +2225,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
u32 page_size, temp;
int i;

- INIT_LIST_HEAD(&xhci->cancel_cmd_list);
INIT_LIST_HEAD(&xhci->cmd_list);

page_size = xhci_readl(xhci, &xhci->op_regs->page_size);
@@ -2414,6 +2410,11 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
"Wrote ERST address to ir_set 0.");
xhci_print_ir_set(xhci, 0);

+ /* init command timeout timer */
+ init_timer(&xhci->cmd_timer);
+ xhci->cmd_timer.data = (unsigned long) xhci;
+ xhci->cmd_timer.function = xhci_handle_command_timeout;
+
/*
* XXX: Might need to set the Interrupter Moderation Register to
* something other than the default (~1ms minimum between interrupts).
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6b61787..ea8c60d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -329,71 +329,6 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
return 0;
}

-static int xhci_queue_cd(struct xhci_hcd *xhci,
- struct xhci_command *command,
- union xhci_trb *cmd_trb)
-{
- struct xhci_cd *cd;
- cd = kzalloc(sizeof(struct xhci_cd), GFP_ATOMIC);
- if (!cd)
- return -ENOMEM;
- INIT_LIST_HEAD(&cd->cancel_cmd_list);
-
- cd->command = command;
- cd->cmd_trb = cmd_trb;
- list_add_tail(&cd->cancel_cmd_list, &xhci->cancel_cmd_list);
-
- return 0;
-}
-
-/*
- * Cancel the command which has issue.
- *
- * Some commands may hang due to waiting for acknowledgement from
- * usb device. It is outside of the xHC's ability to control and
- * will cause the command ring is blocked. When it occurs software
- * should intervene to recover the command ring.
- * See Section 4.6.1.1 and 4.6.1.2
- */
-int xhci_cancel_cmd(struct xhci_hcd *xhci, struct xhci_command *command,
- union xhci_trb *cmd_trb)
-{
- int retval = 0;
- unsigned long flags;
-
- spin_lock_irqsave(&xhci->lock, flags);
-
- if (xhci->xhc_state & XHCI_STATE_DYING) {
- xhci_warn(xhci, "Abort the command ring,"
- " but the xHCI is dead.\n");
- retval = -ESHUTDOWN;
- goto fail;
- }
-
- /* queue the cmd desriptor to cancel_cmd_list */
- retval = xhci_queue_cd(xhci, command, cmd_trb);
- if (retval) {
- xhci_warn(xhci, "Queuing command descriptor failed.\n");
- goto fail;
- }
-
- /* abort command ring */
- retval = xhci_abort_cmd_ring(xhci);
- if (retval) {
- xhci_err(xhci, "Abort command ring failed\n");
- if (unlikely(retval == -ESHUTDOWN)) {
- spin_unlock_irqrestore(&xhci->lock, flags);
- usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
- xhci_dbg(xhci, "xHCI host controller is dead.\n");
- return retval;
- }
- }
-
-fail:
- spin_unlock_irqrestore(&xhci->lock, flags);
- return retval;
-}
-
void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
unsigned int slot_id,
unsigned int ep_index,
@@ -1180,164 +1115,6 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
}
}

-/* Complete the command and detele it from the devcie's command queue.
- */
-static void xhci_complete_cmd_in_cmd_wait_list(struct xhci_hcd *xhci,
- struct xhci_command *command, u32 status)
-{
- command->status = status;
- list_del(&command->cmd_list);
- if (command->completion)
- complete(command->completion);
- else
- xhci_free_command(xhci, command);
-}
-
-
-/*
- * Finding the command trb need to be cancelled and modifying it to
- * NO OP command. And if the command is in device's command wait
- * list, finishing and freeing it.
- *
- * If we can't find the command trb, we think it had already been
- * executed.
- */
-static void xhci_cmd_to_noop(struct xhci_hcd *xhci, struct xhci_cd *cur_cd)
-{
- struct xhci_segment *cur_seg;
- union xhci_trb *cmd_trb;
- u32 cycle_state;
-
- if (xhci->cmd_ring->dequeue == xhci->cmd_ring->enqueue)
- return;
-
- /* find the current segment of command ring */
- cur_seg = find_trb_seg(xhci->cmd_ring->first_seg,
- xhci->cmd_ring->dequeue, &cycle_state);
-
- if (!cur_seg) {
- xhci_warn(xhci, "Command ring mismatch, dequeue = %p %llx (dma)\n",
- xhci->cmd_ring->dequeue,
- (unsigned long long)
- xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg,
- xhci->cmd_ring->dequeue));
- xhci_debug_ring(xhci, xhci->cmd_ring);
- xhci_dbg_ring_ptrs(xhci, xhci->cmd_ring);
- return;
- }
-
- /* find the command trb matched by cd from command ring */
- for (cmd_trb = xhci->cmd_ring->dequeue;
- cmd_trb != xhci->cmd_ring->enqueue;
- next_trb(xhci, xhci->cmd_ring, &cur_seg, &cmd_trb)) {
- /* If the trb is link trb, continue */
- if (TRB_TYPE_LINK_LE32(cmd_trb->generic.field[3]))
- continue;
-
- if (cur_cd->cmd_trb == cmd_trb) {
-
- /* If the command in device's command list, we should
- * finish it and free the command structure.
- */
- if (cur_cd->command)
- xhci_complete_cmd_in_cmd_wait_list(xhci,
- cur_cd->command, COMP_CMD_STOP);
-
- /* get cycle state from the origin command trb */
- cycle_state = le32_to_cpu(cmd_trb->generic.field[3])
- & TRB_CYCLE;
-
- /* modify the command trb to NO OP command */
- cmd_trb->generic.field[0] = 0;
- cmd_trb->generic.field[1] = 0;
- cmd_trb->generic.field[2] = 0;
- cmd_trb->generic.field[3] = cpu_to_le32(
- TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
- break;
- }
- }
-}
-
-static void xhci_cancel_cmd_in_cd_list(struct xhci_hcd *xhci)
-{
- struct xhci_cd *cur_cd, *next_cd;
-
- if (list_empty(&xhci->cancel_cmd_list))
- return;
-
- list_for_each_entry_safe(cur_cd, next_cd,
- &xhci->cancel_cmd_list, cancel_cmd_list) {
- xhci_cmd_to_noop(xhci, cur_cd);
- list_del(&cur_cd->cancel_cmd_list);
- kfree(cur_cd);
- }
-}
-
-/*
- * traversing the cancel_cmd_list. If the command descriptor according
- * to cmd_trb is found, the function free it and return 1, otherwise
- * return 0.
- */
-static int xhci_search_cmd_trb_in_cd_list(struct xhci_hcd *xhci,
- union xhci_trb *cmd_trb)
-{
- struct xhci_cd *cur_cd, *next_cd;
-
- if (list_empty(&xhci->cancel_cmd_list))
- return 0;
-
- list_for_each_entry_safe(cur_cd, next_cd,
- &xhci->cancel_cmd_list, cancel_cmd_list) {
- if (cur_cd->cmd_trb == cmd_trb) {
- if (cur_cd->command)
- xhci_complete_cmd_in_cmd_wait_list(xhci,
- cur_cd->command, COMP_CMD_STOP);
- list_del(&cur_cd->cancel_cmd_list);
- kfree(cur_cd);
- return 1;
- }
- }
-
- return 0;
-}
-
-/*
- * If the cmd_trb_comp_code is COMP_CMD_ABORT, we just check whether the
- * trb pointed by the command ring dequeue pointer is the trb we want to
- * cancel or not. And if the cmd_trb_comp_code is COMP_CMD_STOP, we will
- * traverse the cancel_cmd_list to trun the all of the commands according
- * to command descriptor to NO-OP trb.
- */
-static int handle_stopped_cmd_ring(struct xhci_hcd *xhci,
- int cmd_trb_comp_code)
-{
- int cur_trb_is_good = 0;
-
- /* Searching the cmd trb pointed by the command ring dequeue
- * pointer in command descriptor list. If it is found, free it.
- */
- cur_trb_is_good = xhci_search_cmd_trb_in_cd_list(xhci,
- xhci->cmd_ring->dequeue);
-
- if (cmd_trb_comp_code == COMP_CMD_ABORT)
- xhci->cmd_ring_state = CMD_RING_STATE_STOPPED;
- else if (cmd_trb_comp_code == COMP_CMD_STOP) {
- /* traversing the cancel_cmd_list and canceling
- * the command according to command descriptor
- */
- xhci_cancel_cmd_in_cd_list(xhci);
-
- xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
- /*
- * ring command ring doorbell again to restart the
- * command ring
- */
- if (xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue)
- xhci_ring_cmd_db(xhci);
- }
- return cur_trb_is_good;
-}
-
static void xhci_handle_cmd_enable_slot(struct xhci_hcd *xhci, int slot_id,
u32 cmd_comp_code)
{
@@ -1435,6 +1212,30 @@ static void xhci_handle_cmd_nec_get_fw(struct xhci_hcd *xhci,
NEC_FW_MINOR(le32_to_cpu(event->status)));
}

+void xhci_handle_command_timeout(unsigned long data)
+{
+ struct xhci_hcd *xhci;
+ int ret;
+ unsigned long flags;
+ xhci = (struct xhci_hcd *) data;
+ /* mark this command to be cancelled */
+ spin_lock_irqsave(&xhci->lock, flags);
+ if (xhci->current_cmd)
+ xhci->current_cmd->status = COMP_CMD_ABORT;
+ spin_unlock_irqrestore(&xhci->lock, flags);
+
+ ret = xhci_abort_cmd_ring(xhci);
+
+ if (ret) {
+ xhci_err(xhci, "Abort command ring failed\n");
+ if (unlikely(ret == -ESHUTDOWN)) {
+ usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
+ xhci_dbg(xhci, "xHCI host controller is dead.\n");
+ }
+ }
+ return;
+}
+
static void handle_cmd_completion(struct xhci_hcd *xhci,
struct xhci_event_cmd *event)
{
@@ -1468,26 +1269,64 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
"Command completion event does not match command\n");
return;
}
+
+ del_timer(&xhci->cmd_timer);
+
trace_xhci_cmd_completion(cmd_trb, (struct xhci_generic_trb *) event);

cmd_comp_code = GET_COMP_CODE(le32_to_cpu(event->status));
- if (cmd_comp_code == COMP_CMD_ABORT || cmd_comp_code == COMP_CMD_STOP) {
- /* If the return value is 0, we think the trb pointed by
- * command ring dequeue pointer is a good trb. The good
- * trb means we don't want to cancel the trb, but it have
- * been stopped by host. So we should handle it normally.
- * Otherwise, driver should invoke inc_deq() and return.
- */
- if (handle_stopped_cmd_ring(xhci, cmd_comp_code)) {
- inc_deq(xhci, xhci->cmd_ring);
- return;
+
+ /* If CMD ring stopped we own the trbs between enqueue and dequeue */
+ if (cmd_comp_code == COMP_CMD_STOP) {
+ struct xhci_command *cur_cmd, *tmp_cmd;
+ u32 cycle_state;
+
+ /* Turn all aborted commands in list to no-ops, then restart */
+ list_for_each_entry_safe(cur_cmd, tmp_cmd, &xhci->cmd_list,
+ cmd_list) {
+
+ if (cur_cmd->status != COMP_CMD_ABORT)
+ continue;
+
+ cur_cmd->status = COMP_CMD_STOP;
+
+ /* get cycle state from the original cmd trb */
+ cycle_state = le32_to_cpu(
+ cur_cmd->command_trb->generic.field[3]) &
+ TRB_CYCLE;
+
+ /* modify the command trb to NO OP command */
+ cur_cmd->command_trb->generic.field[0] = 0;
+ cur_cmd->command_trb->generic.field[1] = 0;
+ cur_cmd->command_trb->generic.field[2] = 0;
+ cur_cmd->command_trb->generic.field[3] = cpu_to_le32(
+ TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
+
+ /* completion is called when command completion
+ * event is received for these no-op commands
+ */
}
- /* There is no command to handle if we get a stop event when the
- * command ring is empty, event->cmd_trb points to the next
- * unset command
- */
- if (xhci->cmd_ring->dequeue == xhci->cmd_ring->enqueue)
- return;
+ xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
+
+ /* ring command ring doorbell to restart the command ring */
+ if (xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) {
+ xhci->current_cmd = cmd;
+ mod_timer(&xhci->cmd_timer,
+ jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_ring_cmd_db(xhci);
+ }
+ return;
+ }
+ /*
+ * Host aborted the command ring, check if the current command was
+ * supposed to be aborted, otherwise continue normally.
+ * The command ring is stopped now, but the xHC will issue a Command
+ * Ring Stopped event which will cause us to restart it.
+ */
+ if (cmd_comp_code == COMP_CMD_ABORT) {
+ xhci->cmd_ring_state = CMD_RING_STATE_STOPPED;
+ if (cmd->status == COMP_CMD_ABORT)
+ goto event_handled;
}

cmd_type = TRB_FIELD_TO_TYPE(le32_to_cpu(cmd_trb->generic.field[3]));
@@ -1518,6 +1357,9 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
xhci_handle_cmd_set_deq(xhci, slot_id, cmd_trb, cmd_comp_code);
break;
case TRB_CMD_NOOP:
+ /* Is this an aborted command turned to NO-OP? */
+ if (cmd->status == COMP_CMD_STOP)
+ cmd_comp_code = COMP_CMD_STOP;
break;
case TRB_RESET_EP:
WARN_ON(slot_id != TRB_TO_SLOT_ID(
@@ -1538,6 +1380,14 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
break;
}

+ /* restart timer if this wasn't the last command */
+ if (cmd->cmd_list.next != &xhci->cmd_list) {
+ xhci->current_cmd = list_entry(cmd->cmd_list.next,
+ struct xhci_command, cmd_list);
+ mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
+ }
+
+event_handled:
list_del(&cmd->cmd_list);

if (cmd->completion) {
@@ -3950,6 +3800,14 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
}
cmd->command_trb = xhci->cmd_ring->enqueue;
list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
+
+ /* if there are no other commands queued we start the timeout timer */
+ if (xhci->cmd_list.next == &cmd->cmd_list &&
+ !timer_pending(&xhci->cmd_timer)) {
+ xhci->current_cmd = cmd;
+ mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
+ }
+
queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
field4 | xhci->cmd_ring->cycle_state);
return 0;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e02c80e..e2c1474 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1812,6 +1812,11 @@ static int xhci_configure_endpoint_result(struct xhci_hcd *xhci,
int ret;

switch (*cmd_status) {
+ case COMP_CMD_ABORT:
+ case COMP_CMD_STOP:
+ xhci_warn(xhci, "Timeout while waiting for configure endpoint command\n");
+ ret = -ETIME;
+ break;
case COMP_ENOMEM:
dev_warn(&udev->dev, "Not enough host controller resources "
"for new device state.\n");
@@ -1858,6 +1863,11 @@ static int xhci_evaluate_context_result(struct xhci_hcd *xhci,
struct xhci_virt_device *virt_dev = xhci->devs[udev->slot_id];

switch (*cmd_status) {
+ case COMP_CMD_ABORT:
+ case COMP_CMD_STOP:
+ xhci_warn(xhci, "Timeout while waiting for evaluate context command\n");
+ ret = -ETIME;
+ break;
case COMP_EINVAL:
dev_warn(&udev->dev, "WARN: xHCI driver setup invalid evaluate "
"context command.\n");
@@ -2582,7 +2592,6 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
bool ctx_change, bool must_succeed)
{
int ret;
- int timeleft;
unsigned long flags;
struct xhci_input_control_ctx *ctrl_ctx;
struct xhci_virt_device *virt_dev;
@@ -2638,21 +2647,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
spin_unlock_irqrestore(&xhci->lock, flags);

/* Wait for the configure endpoint command to complete */
- timeleft = wait_for_completion_interruptible_timeout(
- command->completion,
- XHCI_CMD_DEFAULT_TIMEOUT);
- if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for %s command\n",
- timeleft == 0 ? "Timeout" : "Signal",
- ctx_change == 0 ?
- "configure endpoint" :
- "evaluate context");
- /* cancel the configure endpoint command */
- ret = xhci_cancel_cmd(xhci, command, command->command_trb);
- if (ret < 0)
- return ret;
- return -ETIME;
- }
+ wait_for_completion(command->completion);

if (!ctx_change)
ret = xhci_configure_endpoint_result(xhci, udev,
@@ -3408,7 +3403,6 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
unsigned int slot_id;
struct xhci_virt_device *virt_dev;
struct xhci_command *reset_device_cmd;
- int timeleft;
int last_freed_endpoint;
struct xhci_slot_ctx *slot_ctx;
int old_active_eps = 0;
@@ -3476,15 +3470,7 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
spin_unlock_irqrestore(&xhci->lock, flags);

/* Wait for the Reset Device command to finish */
- timeleft = wait_for_completion_interruptible_timeout(
- reset_device_cmd->completion,
- XHCI_CMD_DEFAULT_TIMEOUT);
- if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for reset device command\n",
- timeleft == 0 ? "Timeout" : "Signal");
- ret = -ETIME;
- goto command_cleanup;
- }
+ wait_for_completion(reset_device_cmd->completion);

/* The Reset Device command can't fail, according to the 0.95/0.96 spec,
* unless we tried to reset a slot ID that wasn't enabled,
@@ -3492,6 +3478,11 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
*/
ret = reset_device_cmd->status;
switch (ret) {
+ case COMP_CMD_ABORT:
+ case COMP_CMD_STOP:
+ xhci_warn(xhci, "Timeout waiting for reset device command\n");
+ ret = -ETIME;
+ goto command_cleanup;
case COMP_EBADSLT: /* 0.95 completion code for bad slot ID */
case COMP_CTX_STATE: /* 0.96 completion code for same thing */
xhci_dbg(xhci, "Can't reset device (slot ID %u) in %s state\n",
@@ -3659,7 +3650,6 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
unsigned long flags;
- int timeleft;
int ret;
struct xhci_command *command;

@@ -3679,19 +3669,9 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);

- /* XXX: how much time for xHC slot assignment? */
- timeleft = wait_for_completion_interruptible_timeout(
- command->completion,
- XHCI_CMD_DEFAULT_TIMEOUT);
- if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for a slot\n",
- timeleft == 0 ? "Timeout" : "Signal");
- /* cancel the enable slot request */
- ret = xhci_cancel_cmd(xhci, NULL, command->command_trb);
- return ret;
- }
+ wait_for_completion(command->completion);

- if (!xhci->slot_id) {
+ if (!xhci->slot_id || command->status != COMP_SUCCESS) {
xhci_err(xhci, "Error while assigning device slot ID\n");
kfree(command);
return 0;
@@ -3755,7 +3735,6 @@ disable_slot:
int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
{
unsigned long flags;
- int timeleft;
struct xhci_virt_device *virt_dev;
int ret = 0;
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
@@ -3830,24 +3809,18 @@ int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
spin_unlock_irqrestore(&xhci->lock, flags);

/* ctrl tx can take up to 5 sec; XXX: need more time for xHC? */
- timeleft = wait_for_completion_interruptible_timeout(
- command->completion,
- XHCI_CMD_DEFAULT_TIMEOUT);
+ wait_for_completion(command->completion);
+
/* FIXME: From section 4.3.4: "Software shall be responsible for timing
* the SetAddress() "recovery interval" required by USB and aborting the
* command on a timeout.
*/
- if (timeleft <= 0) {
- xhci_warn(xhci, "%s while waiting for address device command\n",
- timeleft == 0 ? "Timeout" : "Signal");
- /* cancel the address device command */
- ret = xhci_cancel_cmd(xhci, NULL, command->command_trb);
- if (ret < 0)
- return ret;
- return -ETIME;
- }
-
switch (command->status) {
+ case COMP_CMD_ABORT:
+ case COMP_CMD_STOP:
+ xhci_warn(xhci, "Timeout while waiting for address device command\n");
+ ret = -ETIME;
+ break;
case COMP_CTX_STATE:
case COMP_EBADSLT:
xhci_err(xhci, "Setup ERROR: address device command for slot %d.\n",
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index afff1da..14da9fc 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1287,7 +1287,6 @@ struct xhci_td {

/* command descriptor */
struct xhci_cd {
- struct list_head cancel_cmd_list;
struct xhci_command *command;
union xhci_trb *cmd_trb;
};
@@ -1471,9 +1470,10 @@ struct xhci_hcd {
#define CMD_RING_STATE_RUNNING (1 << 0)
#define CMD_RING_STATE_ABORTED (1 << 1)
#define CMD_RING_STATE_STOPPED (1 << 2)
- struct list_head cancel_cmd_list;
struct list_head cmd_list;
unsigned int cmd_ring_reserved_trbs;
+ struct timer_list cmd_timer;
+ struct xhci_command *current_cmd;
struct xhci_ring *event_ring;
struct xhci_erst erst;
/* Scratchpad */
@@ -1849,8 +1849,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);
-int xhci_cancel_cmd(struct xhci_hcd *xhci, struct xhci_command *command,
- union xhci_trb *cmd_trb);
+void xhci_handle_command_timeout(unsigned long data);
+
void xhci_ring_ep_doorbell(struct xhci_hcd *xhci, unsigned int slot_id,
unsigned int ep_index, unsigned int stream_id);

--
1.8.1.2

2014-01-30 14:05:54

by Mathias Nyman

Subject: [RFCv2 04/10] xhci: use command structures for xhci_queue_stop_endpoint()

Prepare for the global command ring by using command structures
internally in functions calling xhci_queue_stop_endpoint()

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-hub.c | 15 +++++++++++++--
drivers/usb/host/xhci.c | 3 +++
2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 805f234..68bc1c6 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -20,7 +20,8 @@
* Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/

-#include <linux/gfp.h>
+
+#include <linux/slab.h>
#include <asm/unaligned.h>

#include "xhci.h"
@@ -284,8 +285,18 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)

spin_lock_irqsave(&xhci->lock, flags);
for (i = LAST_EP_INDEX; i > 0; i--) {
- if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue)
+ if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) {
+ struct xhci_command *command;
+ command = xhci_alloc_command(xhci, false, false,
+ GFP_NOIO);
+ if (!command) {
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ xhci_free_command(xhci, cmd);
+ return -ENOMEM;
+ }
xhci_queue_stop_endpoint(xhci, slot_id, i, suspend);
+ kfree(command);
+ }
}
cmd->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
list_add_tail(&cmd->cmd_list, &virt_dev->cmd_list);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 4ce8c85..d4684d0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1466,6 +1466,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
unsigned int ep_index;
struct xhci_ring *ep_ring;
struct xhci_virt_ep *ep;
+ struct xhci_command *command;

xhci = hcd_to_xhci(hcd);
spin_lock_irqsave(&xhci->lock, flags);
@@ -1535,6 +1536,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
* the first cancellation to be handled.
*/
if (!(ep->ep_state & EP_HALT_PENDING)) {
+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
ep->ep_state |= EP_HALT_PENDING;
ep->stop_cmds_pending++;
ep->stop_cmd_timer.expires = jiffies +
@@ -1542,6 +1544,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
add_timer(&ep->stop_cmd_timer);
xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index, 0);
xhci_ring_cmd_db(xhci);
+ kfree(command);
}
done:
spin_unlock_irqrestore(&xhci->lock, flags);
--
1.8.1.2

2014-01-30 14:06:23

by Mathias Nyman

Subject: [RFCv2 03/10] xhci: use command structures for xhci_queue_slot_control()

Preparing for the global command queue by changing functions calling
xhci_queue_slot_control() to internally use command structures

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci.c | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5bf0f94..4ce8c85 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3564,6 +3564,11 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
unsigned long flags;
u32 state;
int i, ret;
+ struct xhci_command *command;
+
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ if (!command)
+ return;

#ifndef CONFIG_USB_DEFAULT_PERSIST
/*
@@ -3579,8 +3584,10 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
/* If the host is halted due to driver unload, we still need to free the
* device.
*/
- if (ret <= 0 && ret != -ENODEV)
+ if (ret <= 0 && ret != -ENODEV) {
+ kfree(command);
return;
+ }

virt_dev = xhci->devs[udev->slot_id];

@@ -3597,16 +3604,20 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
(xhci->xhc_state & XHCI_STATE_HALTED)) {
xhci_free_virt_device(xhci, udev->slot_id);
spin_unlock_irqrestore(&xhci->lock, flags);
+ kfree(command);
return;
}

if (xhci_queue_slot_control(xhci, TRB_DISABLE_SLOT, udev->slot_id)) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+ kfree(command);
return;
}
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
+ kfree(command);
+
/*
* Event command completion handler will free any data structures
* associated with the slot. XXX Can free sleep?
@@ -3646,31 +3657,41 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
unsigned long flags;
int timeleft;
int ret;
- union xhci_trb *cmd_trb;
+ struct xhci_command *command;
+
+ command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
+ if (!command)
+ return 0;

spin_lock_irqsave(&xhci->lock, flags);
- cmd_trb = xhci_find_next_enqueue(xhci->cmd_ring);
+ command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
+ command->completion = &xhci->addr_dev;
ret = xhci_queue_slot_control(xhci, TRB_ENABLE_SLOT, 0);
if (ret) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
+ kfree(command);
return 0;
}
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);

/* XXX: how much time for xHC slot assignment? */
- timeleft = wait_for_completion_interruptible_timeout(&xhci->addr_dev,
+ timeleft = wait_for_completion_interruptible_timeout(
+ command->completion,
XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for a slot\n",
timeleft == 0 ? "Timeout" : "Signal");
/* cancel the enable slot request */
- return xhci_cancel_cmd(xhci, NULL, cmd_trb);
+ ret = xhci_cancel_cmd(xhci, NULL, command->command_trb);
+ kfree(command);
+ return ret;
}

if (!xhci->slot_id) {
xhci_err(xhci, "Error while assigning device slot ID\n");
+ kfree(command);
return 0;
}

@@ -3705,6 +3726,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
pm_runtime_get_noresume(hcd->self.controller);
#endif

+
+ kfree(command);
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
@@ -3715,6 +3738,7 @@ disable_slot:
if (!xhci_queue_slot_control(xhci, TRB_DISABLE_SLOT, udev->slot_id))
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
+ kfree(command);
return 0;
}

--
1.8.1.2

2014-01-30 14:06:47

by Mathias Nyman

Subject: [RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint

To create a global command queue we need to fill a command structure
for each entry on the command ring.

We start by requiring xhci_configure_endpoint() to be called with
a proper command structure. Functions xhci_check_maxpacket and xhci_check_bandwith
that called xhci_configure_endpoint without a command strucure are fixed.

A special case where an endpoint needs to be configured after reset, done in interrupt
context is also changed to create a command strucure. (this command
structure is not used yet, but will be later when we requre all queued commands
to have a command strucure)

Signed-off-by: Mathias Nyman <[email protected]>
---
drivers/usb/host/xhci-ring.c | 10 ++---
drivers/usb/host/xhci.c | 97 ++++++++++++++++++++++++--------------------
2 files changed, 56 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1e2f3f4..da83a844 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1180,12 +1180,15 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
* because the HW can't handle two commands being queued in a row.
*/
if (xhci->quirks & XHCI_RESET_EP_QUIRK) {
+ struct xhci_command *command;
+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
"Queueing configure endpoint command");
xhci_queue_configure_endpoint(xhci,
xhci->devs[slot_id]->in_ctx->dma, slot_id,
false);
xhci_ring_cmd_db(xhci);
+ kfree(command);
} else {
/* Clear our internal halted state and restart the ring(s) */
xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_HALTED;
@@ -1439,7 +1442,7 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
add_flags - SLOT_FLAG == drop_flags) {
ep_state = virt_dev->eps[ep_index].ep_state;
if (!(ep_state & EP_HALTED))
- goto bandwidth_change;
+ return;
xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
"Completed config ep cmd - "
"last ep index = %d, state = %d",
@@ -1449,11 +1452,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
return;
}
-bandwidth_change:
- xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
- "Completed config ep cmd");
- virt_dev->cmd_status = cmd_comp_code;
- complete(&virt_dev->cmd_completion);
return;
}

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 4265b48..a40485e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1179,10 +1179,10 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
unsigned int ep_index, struct urb *urb)
{
- struct xhci_container_ctx *in_ctx;
struct xhci_container_ctx *out_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
struct xhci_ep_ctx *ep_ctx;
+ struct xhci_command *command;
int max_packet_size;
int hw_max_packet_size;
int ret = 0;
@@ -1207,18 +1207,24 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
/* FIXME: This won't work if a non-default control endpoint
* changes max packet sizes.
*/
- in_ctx = xhci->devs[slot_id]->in_ctx;
- ctrl_ctx = xhci_get_input_control_ctx(xhci, in_ctx);
+
+ command = xhci_alloc_command(xhci, false, true, GFP_KERNEL);
+ if (!command)
+ return -ENOMEM;
+
+ command->in_ctx = xhci->devs[slot_id]->in_ctx;
+ ctrl_ctx = xhci_get_input_control_ctx(xhci, command->in_ctx);
if (!ctrl_ctx) {
xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
__func__);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto command_cleanup;
}
/* Set up the modified control endpoint 0 */
xhci_endpoint_copy(xhci, xhci->devs[slot_id]->in_ctx,
xhci->devs[slot_id]->out_ctx, ep_index);

- ep_ctx = xhci_get_ep_ctx(xhci, in_ctx, ep_index);
+ ep_ctx = xhci_get_ep_ctx(xhci, command->in_ctx, ep_index);
ep_ctx->ep_info2 &= cpu_to_le32(~MAX_PACKET_MASK);
ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet_size));

@@ -1226,17 +1232,20 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
ctrl_ctx->drop_flags = 0;

xhci_dbg(xhci, "Slot %d input context\n", slot_id);
- xhci_dbg_ctx(xhci, in_ctx, ep_index);
+ xhci_dbg_ctx(xhci, command->in_ctx, ep_index);
xhci_dbg(xhci, "Slot %d output context\n", slot_id);
xhci_dbg_ctx(xhci, out_ctx, ep_index);

- ret = xhci_configure_endpoint(xhci, urb->dev, NULL,
+ ret = xhci_configure_endpoint(xhci, urb->dev, command,
true, false);

/* Clean up the input context for later use by bandwidth
* functions.
*/
ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG);
+command_cleanup:
+ kfree(command->completion);
+ kfree(command);
}
return ret;
}
@@ -2568,21 +2577,16 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
int ret;
int timeleft;
unsigned long flags;
- struct xhci_container_ctx *in_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
- struct completion *cmd_completion;
- u32 *cmd_status;
struct xhci_virt_device *virt_dev;
- union xhci_trb *cmd_trb;
+
+ if (!command)
+ return -EINVAL;

spin_lock_irqsave(&xhci->lock, flags);
virt_dev = xhci->devs[udev->slot_id];

- if (command)
- in_ctx = command->in_ctx;
- else
- in_ctx = virt_dev->in_ctx;
- ctrl_ctx = xhci_get_input_control_ctx(xhci, in_ctx);
+ ctrl_ctx = xhci_get_input_control_ctx(xhci, command->in_ctx);
if (!ctrl_ctx) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
@@ -2599,7 +2603,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
return -ENOMEM;
}
if ((xhci->quirks & XHCI_SW_BW_CHECKING) &&
- xhci_reserve_bandwidth(xhci, virt_dev, in_ctx)) {
+ xhci_reserve_bandwidth(xhci, virt_dev, command->in_ctx)) {
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK))
xhci_free_host_resources(xhci, ctrl_ctx);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -2607,27 +2611,17 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
return -ENOMEM;
}

- if (command) {
- cmd_completion = command->completion;
- cmd_status = &command->status;
- command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
- list_add_tail(&command->cmd_list, &virt_dev->cmd_list);
- } else {
- cmd_completion = &virt_dev->cmd_completion;
- cmd_status = &virt_dev->cmd_status;
- }
- init_completion(cmd_completion);
+ command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
+ list_add_tail(&command->cmd_list, &virt_dev->cmd_list);

- cmd_trb = xhci_find_next_enqueue(xhci->cmd_ring);
if (!ctx_change)
- ret = xhci_queue_configure_endpoint(xhci, in_ctx->dma,
+ ret = xhci_queue_configure_endpoint(xhci, command->in_ctx->dma,
udev->slot_id, must_succeed);
else
- ret = xhci_queue_evaluate_context(xhci, in_ctx->dma,
+ ret = xhci_queue_evaluate_context(xhci, command->in_ctx->dma,
udev->slot_id, must_succeed);
if (ret < 0) {
- if (command)
- list_del(&command->cmd_list);
+ list_del(&command->cmd_list);
if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK))
xhci_free_host_resources(xhci, ctrl_ctx);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -2640,7 +2634,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,

/* Wait for the configure endpoint command to complete */
timeleft = wait_for_completion_interruptible_timeout(
- cmd_completion,
+ command->completion,
XHCI_CMD_DEFAULT_TIMEOUT);
if (timeleft <= 0) {
xhci_warn(xhci, "%s while waiting for %s command\n",
@@ -2649,16 +2643,18 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
"configure endpoint" :
"evaluate context");
/* cancel the configure endpoint command */
- ret = xhci_cancel_cmd(xhci, command, cmd_trb);
+ ret = xhci_cancel_cmd(xhci, command, command->command_trb);
if (ret < 0)
return ret;
return -ETIME;
}

if (!ctx_change)
- ret = xhci_configure_endpoint_result(xhci, udev, cmd_status);
+ ret = xhci_configure_endpoint_result(xhci, udev,
+ &command->status);
else
- ret = xhci_evaluate_context_result(xhci, udev, cmd_status);
+ ret = xhci_evaluate_context_result(xhci, udev,
+ &command->status);

if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
spin_lock_irqsave(&xhci->lock, flags);
@@ -2692,6 +2688,7 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
struct xhci_virt_device *virt_dev;
struct xhci_input_control_ctx *ctrl_ctx;
struct xhci_slot_ctx *slot_ctx;
+ struct xhci_command *command;

ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
if (ret <= 0)
@@ -2703,12 +2700,19 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
virt_dev = xhci->devs[udev->slot_id];

+ command = xhci_alloc_command(xhci, false, true, GFP_KERNEL);
+ if (!command)
+ return -ENOMEM;
+
+ command->in_ctx = virt_dev->in_ctx;
+
/* See section 4.6.6 - A0 = 1; A1 = D0 = D1 = 0 */
- ctrl_ctx = xhci_get_input_control_ctx(xhci, virt_dev->in_ctx);
+ ctrl_ctx = xhci_get_input_control_ctx(xhci, command->in_ctx);
if (!ctrl_ctx) {
xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
__func__);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto command_cleanup;
}
ctrl_ctx->add_flags |= cpu_to_le32(SLOT_FLAG);
ctrl_ctx->add_flags &= cpu_to_le32(~EP0_FLAG);
@@ -2716,20 +2720,20 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)

/* Don't issue the command if there's no endpoints to update. */
if (ctrl_ctx->add_flags == cpu_to_le32(SLOT_FLAG) &&
- ctrl_ctx->drop_flags == 0)
- return 0;
-
+ ctrl_ctx->drop_flags == 0) {
+ ret = 0;
+ goto command_cleanup;
+ }
xhci_dbg(xhci, "New Input Control Context:\n");
slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
xhci_dbg_ctx(xhci, virt_dev->in_ctx,
LAST_CTX_TO_EP_NUM(le32_to_cpu(slot_ctx->dev_info)));

- ret = xhci_configure_endpoint(xhci, udev, NULL,
+ ret = xhci_configure_endpoint(xhci, udev, command,
false, false);
- if (ret) {
+ if (ret)
/* Callee should call reset_bandwidth() */
- return ret;
- }
+ goto command_cleanup;

xhci_dbg(xhci, "Output context after successful config ep cmd:\n");
xhci_dbg_ctx(xhci, virt_dev->out_ctx,
@@ -2758,6 +2762,9 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
virt_dev->eps[i].ring = virt_dev->eps[i].new_ring;
virt_dev->eps[i].new_ring = NULL;
}
+command_cleanup:
+ kfree(command->completion);
+ kfree(command);

return ret;
}
--
1.8.1.2

2014-01-30 14:26:19

by David Laight

Subject: RE: [RFCv2 00/10] xhci: re-work command queue management

From: Mathias Nyman
> Only changes since v1 are fixing smatch warnings and errors.
> patch 01/10
> Check for null return from alloc_command, release lock in error path and
> don't dereference possible null pointer in error path.
>
> patch 04/10
> release lock in xhci_stop_dev() error path.
>
> This is the second attempt to re-work and solve the issues in xhci command
> queue management that Sarah has descibed earlier:
>
> Right now, the command management in the xHCI driver is rather ad-hock.
> Different parts of the driver all submit commands, including interrupt
> handling routines, functions called from the USB core (with or without the
> bus bandwidth mutex held).
> Some times they need to wait for the command to complete, and sometimes
> they just issue the command and don't care about the result of the command.
>
> The places that wait on a command all time the command for five seconds,
> and then attempt to cancel the command.
> Unfortunately, that means if several commands are issued at once, and one of
> them times out, all the commands timeout, even though the host hasn't gotten
> a chance to service them yet.
>
> This is apparent with some devices that take a long time to respond to the
> Set Address command during device enumeration (when the device is plugged in).
> If a driver for a different device attempts to change alternate interface
> settings at the same time (causing a Configure Endpoint command to be issued),
> both commands timeout.
>
> Instead of having each command timeout after five seconds, the driver should
> wait indefinitely in an uninterruptible sleep on the command completion.
> A global command queue manager should time whatever command is currently
> running, and cancel that command after five seconds.
>
> If the commands were in a list, like TDs currently are, it may be easier to keep
> track of where the command ring dequeue pointer is, and avoid racing with events.
> We may need to have parts of the driver that issue commands without waiting on
> them still put the commands in the command list.
>
> The Implementation:
> -------------------
>
> First step is to create a list of the commands submitted to the command queue.
> To accomplish this each command is required to be submitted with a properly
> filled command structure containing completion, status variable and a pointer to
> the command TRB that will be used.

I think it would be much simpler to allocate a parallel array to the actual
hardware command ring that contains the additional information for the request
(instead of allocating it pre-request).
This would immediately solve any problems allocating the memory from interrupt
context and failing to free in correctly in all the code paths.

A similar solution could be used for the transfer rings thus removing the
need to the 'td' list - which there are reports of it failing to find transfers
and the code paths for aborting isoch transfers are badly broken.

Adding another list that will have its own set of bugs seems retrograde top me.

David

2014-01-30 22:40:02

by Sarah Sharp

Subject: Re: [RFCv2 00/10] xhci: re-work command queue management

On Thu, Jan 30, 2014 at 02:25:48PM +0000, David Laight wrote:
> I think it would be much simpler to allocate a parallel array to the actual
> hardware command ring that contains the additional information for the request
> (instead of allocating it pre-request).
> This would immediately solve any problems allocating the memory from interrupt
> context and failing to free in correctly in all the code paths.
>
> A similar solution could be used for the transfer rings thus removing the
> need to the 'td' list - which there are reports of it failing to find transfers
> and the code paths for aborting isoch transfers are badly broken.
>
> Adding another list that will have its own set of bugs seems retrograde top me.

I do not have a problem with it. The shadow ring is an optimization we
can look at later.

Sarah Sharp

2014-02-05 02:21:49

by Dan Williams

Subject: Re: [RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint

Hi Mathias, comments below:

On Thu, Jan 30, 2014 at 6:10 AM, Mathias Nyman
<[email protected]> wrote:
> To create a global command queue we need to fill a command structure
> for each entry on the command ring.
>
> We start by requiring xhci_configure_endpoint() to be called with
> a proper command structure. Functions xhci_check_maxpacket and xhci_check_bandwith

s/xhci_check_bandwith/xhci_check_bandwidth/

> that called xhci_configure_endpoint without a command strucure are fixed.

s/strucure/structure/

>
> A special case where an endpoint needs to be configured after reset, done in interrupt
> context is also changed to create a command strucure. (this command

s/strucure/structure/

> structure is not used yet, but will be later when we requre all queued commands

s/requre/require/

> to have a command strucure)

s/strucure/structure/

>
> Signed-off-by: Mathias Nyman <[email protected]>
> ---
> drivers/usb/host/xhci-ring.c | 10 ++---
> drivers/usb/host/xhci.c | 97 ++++++++++++++++++++++++--------------------
> 2 files changed, 56 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1e2f3f4..da83a844 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1180,12 +1180,15 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
> * because the HW can't handle two commands being queued in a row.
> */
> if (xhci->quirks & XHCI_RESET_EP_QUIRK) {
> + struct xhci_command *command;
> + command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);

One cleanup we may want to consider in this series is making
xhci_alloc_command() more readable. My brain hurts when I see "false,
false" as I wonder what that means. I took a look and of the 4
possible ways to call xhci_alloc_command, we only use 2:

$ git grep xhci_alloc_command\(.*\) | grep -o
xhci_alloc_command\(xhci,.*,.*, | sort -u
xhci_alloc_command(xhci, false, true,
xhci_alloc_command(xhci, true, true,

So a first take is to just have a xhci_alloc_command() for "true,
true" and a xhci_alloc_command_no_ctx() for "false, true".

...uh oh, this series adds a usage of:
xhci_alloc_command(xhci, false, false,

...any reason we can't just use something like
xhci_alloc_command_no_ctx() instead?

Actually just make xhci_alloc_command() take an option in_ctx
parameter, when it is NULL xhci_alloc_command will allocate one,
otherwise it will use the passed in one.

> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
> "Queueing configure endpoint command");
> xhci_queue_configure_endpoint(xhci,
> xhci->devs[slot_id]->in_ctx->dma, slot_id,
> false);
> xhci_ring_cmd_db(xhci);
> + kfree(command);

It's not really acceptable to add dead code in a patch. Consider the
case where some of the patches are reverted due to a regression. If,
for example we revert patch 2, the unused infrastructure in patch1
does not get deleted. Patch size minimization is good, but not when
it separates new infrastructure from its first user.

> } else {
> /* Clear our internal halted state and restart the ring(s) */
> xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_HALTED;
> @@ -1439,7 +1442,7 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
> add_flags - SLOT_FLAG == drop_flags) {
> ep_state = virt_dev->eps[ep_index].ep_state;
> if (!(ep_state & EP_HALTED))
> - goto bandwidth_change;
> + return;
> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
> "Completed config ep cmd - "
> "last ep index = %d, state = %d",
> @@ -1449,11 +1452,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id,
> ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
> return;
> }
> -bandwidth_change:
> - xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
> - "Completed config ep cmd");
> - virt_dev->cmd_status = cmd_comp_code;
> - complete(&virt_dev->cmd_completion);
> return;

This change has no description in the change log. What's the reason
for deleting the goto?

> }
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 4265b48..a40485e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1179,10 +1179,10 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
> static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> unsigned int ep_index, struct urb *urb)
> {
> - struct xhci_container_ctx *in_ctx;
> struct xhci_container_ctx *out_ctx;
> struct xhci_input_control_ctx *ctrl_ctx;
> struct xhci_ep_ctx *ep_ctx;
> + struct xhci_command *command;
> int max_packet_size;
> int hw_max_packet_size;
> int ret = 0;
> @@ -1207,18 +1207,24 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> /* FIXME: This won't work if a non-default control endpoint
> * changes max packet sizes.
> */
> - in_ctx = xhci->devs[slot_id]->in_ctx;
> - ctrl_ctx = xhci_get_input_control_ctx(xhci, in_ctx);
> +
> + command = xhci_alloc_command(xhci, false, true, GFP_KERNEL);
> + if (!command)
> + return -ENOMEM;
> +
> + command->in_ctx = xhci->devs[slot_id]->in_ctx;
> + ctrl_ctx = xhci_get_input_control_ctx(xhci, command->in_ctx);

If you change xhci_alloc_command() to take an in_ctx parameter then
you don't need to kill the 'in_ctx' variable or change any lines that
reference the old local in_ctx variable.

> if (!ctrl_ctx) {
> xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
> __func__);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto command_cleanup;
> }
> /* Set up the modified control endpoint 0 */
> xhci_endpoint_copy(xhci, xhci->devs[slot_id]->in_ctx,
> xhci->devs[slot_id]->out_ctx, ep_index);
>
> - ep_ctx = xhci_get_ep_ctx(xhci, in_ctx, ep_index);
> + ep_ctx = xhci_get_ep_ctx(xhci, command->in_ctx, ep_index);
> ep_ctx->ep_info2 &= cpu_to_le32(~MAX_PACKET_MASK);
> ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet_size));
>
> @@ -1226,17 +1232,20 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> ctrl_ctx->drop_flags = 0;
>
> xhci_dbg(xhci, "Slot %d input context\n", slot_id);
> - xhci_dbg_ctx(xhci, in_ctx, ep_index);
> + xhci_dbg_ctx(xhci, command->in_ctx, ep_index);
> xhci_dbg(xhci, "Slot %d output context\n", slot_id);
> xhci_dbg_ctx(xhci, out_ctx, ep_index);
>
> - ret = xhci_configure_endpoint(xhci, urb->dev, NULL,
> + ret = xhci_configure_endpoint(xhci, urb->dev, command,
> true, false);
>
> /* Clean up the input context for later use by bandwidth
> * functions.
> */
> ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG);
> +command_cleanup:
> + kfree(command->completion);
> + kfree(command);
> }
> return ret;
> }
> @@ -2568,21 +2577,16 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
> int ret;
> int timeleft;
> unsigned long flags;
> - struct xhci_container_ctx *in_ctx;
> struct xhci_input_control_ctx *ctrl_ctx;
> - struct completion *cmd_completion;
> - u32 *cmd_status;
> struct xhci_virt_device *virt_dev;
> - union xhci_trb *cmd_trb;
> +
> + if (!command)
> + return -EINVAL;
>
> spin_lock_irqsave(&xhci->lock, flags);
> virt_dev = xhci->devs[udev->slot_id];
>
> - if (command)
> - in_ctx = command->in_ctx;
> - else
> - in_ctx = virt_dev->in_ctx;
> - ctrl_ctx = xhci_get_input_control_ctx(xhci, in_ctx);
> + ctrl_ctx = xhci_get_input_control_ctx(xhci, command->in_ctx);
> if (!ctrl_ctx) {
> spin_unlock_irqrestore(&xhci->lock, flags);
> xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
> @@ -2599,7 +2603,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
> return -ENOMEM;
> }
> if ((xhci->quirks & XHCI_SW_BW_CHECKING) &&
> - xhci_reserve_bandwidth(xhci, virt_dev, in_ctx)) {
> + xhci_reserve_bandwidth(xhci, virt_dev, command->in_ctx)) {
> if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK))
> xhci_free_host_resources(xhci, ctrl_ctx);
> spin_unlock_irqrestore(&xhci->lock, flags);
> @@ -2607,27 +2611,17 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
> return -ENOMEM;
> }
>
> - if (command) {
> - cmd_completion = command->completion;
> - cmd_status = &command->status;
> - command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> - list_add_tail(&command->cmd_list, &virt_dev->cmd_list);
> - } else {
> - cmd_completion = &virt_dev->cmd_completion;
> - cmd_status = &virt_dev->cmd_status;
> - }
> - init_completion(cmd_completion);
> + command->command_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> + list_add_tail(&command->cmd_list, &virt_dev->cmd_list);
>
> - cmd_trb = xhci_find_next_enqueue(xhci->cmd_ring);
> if (!ctx_change)
> - ret = xhci_queue_configure_endpoint(xhci, in_ctx->dma,
> + ret = xhci_queue_configure_endpoint(xhci, command->in_ctx->dma,
> udev->slot_id, must_succeed);
> else
> - ret = xhci_queue_evaluate_context(xhci, in_ctx->dma,
> + ret = xhci_queue_evaluate_context(xhci, command->in_ctx->dma,
> udev->slot_id, must_succeed);
> if (ret < 0) {
> - if (command)
> - list_del(&command->cmd_list);
> + list_del(&command->cmd_list);
> if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK))
> xhci_free_host_resources(xhci, ctrl_ctx);
> spin_unlock_irqrestore(&xhci->lock, flags);
> @@ -2640,7 +2634,7 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
>
> /* Wait for the configure endpoint command to complete */
> timeleft = wait_for_completion_interruptible_timeout(
> - cmd_completion,
> + command->completion,
> XHCI_CMD_DEFAULT_TIMEOUT);
> if (timeleft <= 0) {
> xhci_warn(xhci, "%s while waiting for %s command\n",

Given that we are waiting for the command to finish within
xhci_configure_endpoint() shouldn't we free the completion in
xhci_configure_endpoint as well? In other words in what cases do we
need an xhci_command to have a longer lifetime than the scope of the
execution routine (xhci_stop_device, xhci_configure_endpoint,
xhci_discover_or_reset_device, xhci_alloc_dev, xhci_setup_device).

Taking things a step further it seems that all the locations where we
asynchronously queue commands are in the completion handlers for other
commands. I'm wondering if this would be cleaner if we simply queued
all command submissions and completion events to a single threaded
workqueue. I'll go through the rest of the series to see if that
impression makes sense, but something to consider...

2014-02-05 02:35:04

by Dan Williams

Subject: Re: [RFCv2 00/10] xhci: re-work command queue management

On Thu, Jan 30, 2014 at 6:25 AM, David Laight <[email protected]> wrote:
> From: Mathias Nyman
>> Only changes since v1 are fixing smatch warnings and errors.
>> patch 01/10
>> Check for null return from alloc_command, release lock in error path and
>> don't dereference possible null pointer in error path.
>>
>> patch 04/10
>> release lock in xhci_stop_dev() error path.
>>
>> This is the second attempt to re-work and solve the issues in xhci command
>> queue management that Sarah has descibed earlier:
>>
>> Right now, the command management in the xHCI driver is rather ad-hock.
>> Different parts of the driver all submit commands, including interrupt
>> handling routines, functions called from the USB core (with or without the
>> bus bandwidth mutex held).
>> Some times they need to wait for the command to complete, and sometimes
>> they just issue the command and don't care about the result of the command.
>>
>> The places that wait on a command all time the command for five seconds,
>> and then attempt to cancel the command.
>> Unfortunately, that means if several commands are issued at once, and one of
>> them times out, all the commands timeout, even though the host hasn't gotten
>> a chance to service them yet.
>>
>> This is apparent with some devices that take a long time to respond to the
>> Set Address command during device enumeration (when the device is plugged in).
>> If a driver for a different device attempts to change alternate interface
>> settings at the same time (causing a Configure Endpoint command to be issued),
>> both commands timeout.
>>
>> Instead of having each command timeout after five seconds, the driver should
>> wait indefinitely in an uninterruptible sleep on the command completion.
>> A global command queue manager should time whatever command is currently
>> running, and cancel that command after five seconds.
>>
>> If the commands were in a list, like TDs currently are, it may be easier to keep
>> track of where the command ring dequeue pointer is, and avoid racing with events.
>> We may need to have parts of the driver that issue commands without waiting on
>> them still put the commands in the command list.
>>
>> The Implementation:
>> -------------------
>>
>> First step is to create a list of the commands submitted to the command queue.
>> To accomplish this each command is required to be submitted with a properly
>> filled command structure containing completion, status variable and a pointer to
>> the command TRB that will be used.
>
> I think it would be much simpler to allocate a parallel array to the actual
> hardware command ring that contains the additional information for the request
> (instead of allocating it pre-request).
> This would immediately solve any problems allocating the memory from interrupt
> context and failing to free in correctly in all the code paths.
>
> A similar solution could be used for the transfer rings thus removing the
> need to the 'td' list - which there are reports of it failing to find transfers
> and the code paths for aborting isoch transfers are badly broken.
>
> Adding another list that will have its own set of bugs seems retrograde top me.

What bugs? Please be specific. The problem to be addressed is not
the allocation of commands, but that timeouts of one command eat the
timeout periods of subsequent commands. I'm thinking a
single-threaded workqueue better models what the hardware is doing.
See my comments in patch 1, but that is orthogonal to how the command
contexts are allocated.

2014-02-05 06:57:12

by Dan Williams

Subject: Re: [RFCv2 08/10] xhci: Add a global command queue

On Thu, Jan 30, 2014 at 6:10 AM, Mathias Nyman
<[email protected]> wrote:
> Create a list to store command structures, add a strucure to it every time
> a command is submitted, and remove it from the list once we get a
> command completion event matching the command.
>
> Signed-off-by: Mathias Nyman <[email protected]>
> ---
> drivers/usb/host/xhci-mem.c | 8 ++++++++
> drivers/usb/host/xhci-ring.c | 13 ++++++++++++-
> drivers/usb/host/xhci.h | 1 +
> 3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 49b8bd0..7f8e4c3 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1694,6 +1694,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
> {
> struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> struct xhci_cd *cur_cd, *next_cd;
> + struct xhci_command *cur_cmd, *next_cmd;
> int size;
> int i, j, num_ports;
>
> @@ -1722,6 +1723,12 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
> kfree(cur_cd);
> }
>
> + list_for_each_entry_safe(cur_cmd, next_cmd,
> + &xhci->cmd_list, cmd_list) {
> + list_del(&cur_cmd->cmd_list);
> + kfree(cur_cmd);
> + }
> +

Aren't commands on the cmd_list currently being executed, or are there
other guarantees that make sure all commands have terminated?

2014-02-05 09:23:53

by David Laight

Subject: RE: [RFCv2 00/10] xhci: re-work command queue management

From: Dan Williams
> > Adding another list that will have its own set of bugs seems retrograde top me.
>
> What bugs? Please be specific. The problem to be addressed is not
> the allocation of commands, but that timeouts of one command eat the
> timeout periods of subsequent commands. I'm thinking a
> single-threaded workqueue better models what the hardware is doing.
> See my comments in patch 1, but that is orthogonal to how the command
> contexts are allocated.

No software is bug free.
The best way to get reasonably bug-free software is the KISS principle
(Keep It Simple Stupid).
You just seem to be adding a lot of additional complexity.
Maybe it would look better if more of the code was factored out of the
call sites.

IIRC at the moment every call site has to:
1) find the trb address that will be used (massive layer break).
2) actually write the trb (through several layers of function call).
3) sort out any timeout (I didn't really look into this bit).
4) ring the bell.

ISTM that the call site should just be a single function call.
If you do that the implementation of the timeouts/completions is
removed from the callers.

David

2014-02-05 14:51:24

by Mathias Nyman

Subject: Re: [RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint

On 02/05/2014 04:21 AM, Dan Williams wrote:
> Hi Mathias, comments below:
>
> s/xhci_check_bandwith/xhci_check_bandwidth/
> s/strucure/structure/
> s/strucure/structure/
> s/requre/require/
> s/strucure/structure/
>

Thanks
I guess I need to start using a spell checker for commit messages.

>
> One cleanup we may want to consider in this series is making
> xhci_alloc_command() more readable. My brain hurts when I see "false,
> false" as I wonder what that means. I took a look and of the 4
> possible ways to call xhci_alloc_command, we only use 2:
>
> $ git grep xhci_alloc_command\(.*\) | grep -o
> xhci_alloc_command\(xhci,.*,.*, | sort -u
> xhci_alloc_command(xhci, false, true,
> xhci_alloc_command(xhci, true, true,
>
> So a first take is to just have a xhci_alloc_command() for "true,
> true" and a xhci_alloc_command_no_ctx() for "false, true".
>
> ...uh oh, this series adds a usage of:
> xhci_alloc_command(xhci, false, false,
>
> ...any reason we can't just use something like
> xhci_alloc_command_no_ctx() instead?
>
> Actually just make xhci_alloc_command() take an option in_ctx
> parameter, when it is NULL xhci_alloc_command will allocate one,
> otherwise it will use the passed in one.
>

This would make the code more readable.
The same thing needs to be done for the completion parameter as well then.

Do you think this change would fit in this patch series, or maybe as a
separate fix?

>> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
>> "Queueing configure endpoint command");
>> xhci_queue_configure_endpoint(xhci,
>> xhci->devs[slot_id]->in_ctx->dma, slot_id,
>> false);
>> xhci_ring_cmd_db(xhci);
>> + kfree(command);
>
> It's not really acceptable to add dead code in a patch. Consider the
> case where some of the patches are reverted due to a regression. If,
> for example we revert patch 2, the unused infrastructure in patch1
> does not get deleted. Patch size minimization is good, but not when
> it separates new infrastructure from its first user.

This was a tradeoff I wasn't sure how to do. The first six patches make
sure there exists a command structure every time a command is submitted.
I added the kfrees because I didn't want to leak memory
up to the patch where the command can be freed in its right place (patch
9).

Actually, now looking at it, the command is still not properly freed
between patches 7 and 9.

Any suggestions? squashing first most of the commits together, or just
ignoring that memory is leaked mid-series?

>> -bandwidth_change:
>> - xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
>> - "Completed config ep cmd");
>> - virt_dev->cmd_status = cmd_comp_code;
>> - complete(&virt_dev->cmd_completion);
>> return;
>
> This change has no description in the change log. What's the reason
> for deleting the goto?
>

Previously xhci_configure_endpoint() could also be called without a
command parameter. In this case the completion was _not_ added to
device's own "command wait list". xhci_configure_endpoint() would wait
for completion on xhci->devs[udev->slot_id]->cmd_completion, and
the code after the bandwith_change goto was run.

Now this patch forces all xhci_configure_endpoint() callers to have a
command structure parameter, and now in all cases we're waiting for a
configure endpoint completion, the completion is added to the device's
own "command wait list". These completions are called in the beginning
of handle_cmd_completion_ep by handle_cmd_in_cmd_wait_list().

I probably should add some description about this in the changelog as well.

>
> Given that we are waiting for the command to finish within
> xhci_configure_endpoint() shouldn't we free the completion in
> xhci_configure_endpoint as well? In other words in what cases do we
> need an xhci_command to have a longer lifetime than the scope of the
> execution routine (xhci_stop_device, xhci_configure_endpoint,
> xhci_discover_or_reset_device, xhci_alloc_dev, xhci_setup_device).

Many of the functions that call xhci_configure_endpoint() handle their
command strucure and completion allocation/freeing in their own little
way. I didn't want to mess with these.

For example
xhci_free_streams() uses some pre-allocated command strucure
command = vdev->eps[ep_index].stream_info->free_streams_command;

while xhci_update_hub_device() allocates a new command with completion
before calling xhci_configure_endpoint(), and frees them both afterwards


>
> Taking things a step further it seems that all the locations where we
> asynchronously queue commands are in the completion handlers for other
> commands. I'm wondering if this would be cleaner if we simply queued
> all command submissions and completion events to a single threaded
> workqueue. I'll go through the rest of the series to see if that
> impression makes sense, but something to consider...
>

Handling the command completions in a workqueue could make sense, then
all the async-queued commands could be allocated outside interrupt
context. Not sure if this would expose or create some new races.

I'm not completely sure on what you have in mind when you say you want
to "queue all command submission and completion events to a single
threaded workqueue"

Thanks for taking a look at this

-Mathias

2014-02-05 16:48:30

by Dan Williams

Subject: Re: [RFCv2 00/10] xhci: re-work command queue management

On Wed, Feb 5, 2014 at 1:22 AM, David Laight <[email protected]> wrote:
> From: Dan Williams
>> > Adding another list that will have its own set of bugs seems retrograde top me.
>>
>> What bugs? Please be specific. The problem to be addressed is not
>> the allocation of commands, but that timeouts of one command eat the
>> timeout periods of subsequent commands. I'm thinking a
>> single-threaded workqueue better models what the hardware is doing.
>> See my comments in patch 1, but that is orthogonal to how the command
>> contexts are allocated.
>
> No software is bug free.
> The best way to get reasonably bug-free software is the KISS principle
> (Keep It Simple Stupid).

I find this condescending.

Perhaps you did not mean for it to come out that way, but this is far
from a specific technical argument.

> You just seem to be adding a lot of additional complexity.
> Maybe it would look better if more of the code was factored out of the
> call sites.
>
> IIRC at the moment every call site has to:
> 1) find the trb address that will be used (massive layer break).
> 2) actually write the trb (through several layers of function call).
> 3) sort out any timeout (I didn't really look into this bit).
> 4) ring the bell.

I think there is room to push these pre-requisites down.

> ISTM that the call site should just be a single function call.
> If you do that the implementation of the timeouts/completions is
> removed from the callers.

Yes, but I think we need to centralize the context under which
commands are submitted. The complicating factor is the mix of
synchronous command submission and interrupt driven asynchronous
command queuing. I think we can simplify it by making it all
submitted from a single event queue context. I'm investigating if
that is a workable solution...

2014-02-05 17:06:27

by David Laight

Subject: RE: [RFCv2 00/10] xhci: re-work command queue management

From: Dan Williams
> Yes, but I think we need to centralize the context under which
> commands are submitted. The complicating factor is the mix of
> synchronous command submission and interrupt driven asynchronous
> command queuing. I think we can simplify it by making it all
> submitted from a single event queue context. I'm investigating if
> that is a workable solution...

Given that the entire network stack runs from (I believe) 'process level'
contexts (NAPI functions), it is rather surprising that the USB stack
runs everything from the actual interrupt context (or in the callers
context with interrupts disabled).

For large SMP systems disabling interrupts and acquiring global locks
like that just doesn't scale.

Even for 'normal' URB it ought to be possible to queue them for later
submission - rather than having to immediately put them on the transfer
ring.

Ideally the 'event' ring ought to be large enough for all the events
that the controller can add. This is approximately 1 per TD (2 for
short RX) and one per command. So much like the 'reserved command'
slots (which aren't implemented correctly) there should probably be
'reserved event' slots - otherwise the event ring might become full
(if the host stops servicing interrupts for any length of time.)
Avoiding the need for large numbers of events really means restricting
the number of TD (that request interrupts).

isoc and disk might limit this anyway, but network traffic can queue
a much larger number of TD. I've seen 20 tx TD (all nearly 60kB)
queued. Every time one completes another is immediately added.
usbnet needs to be taught how to do BQL, and something (somehow)
reduce the number of interrupts.

(end of ramble)

David

2014-02-05 17:27:57

by Dan Williams

Subject: Re: [RFCv2 00/10] xhci: re-work command queue management

On Wed, Feb 5, 2014 at 9:05 AM, David Laight <[email protected]> wrote:
> From: Dan Williams
>> Yes, but I think we need to centralize the context under which
>> commands are submitted. The complicating factor is the mix of
>> synchronous command submission and interrupt driven asynchronous
>> command queuing. I think we can simplify it by making it all
>> submitted from a single event queue context. I'm investigating if
>> that is a workable solution...
>
> Given that the entire network stack runs from (I believe) 'process level'
> contexts (NAPI functions),

Not really. Much of the network stack runs under softirq context.

2014-02-05 22:26:20

by Sarah Sharp

Subject: Re: [RFCv2 08/10] xhci: Add a global command queue

On Tue, Feb 04, 2014 at 10:57:09PM -0800, Dan Williams wrote:
> On Thu, Jan 30, 2014 at 6:10 AM, Mathias Nyman <[email protected]> wrote:
> > @@ -1722,6 +1723,12 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
> > kfree(cur_cd);
> > }
> >
> > + list_for_each_entry_safe(cur_cmd, next_cmd,
> > + &xhci->cmd_list, cmd_list) {
> > + list_del(&cur_cmd->cmd_list);
> > + kfree(cur_cmd);
> > + }
> > +
>
> Aren't commands on the cmd_list currently being executed, or are there
> other guarantees that make sure all commands have terminated?

By the time we get to xhci_mem_cleanup, we've done our best effort to
halt the xHCI host controller. That could timeout, I suppose, but I'm
not sure what we're expected to do in that case. If the host won't
halt, I'm not sure it will be able to, say, respond to the request to
cancel the current command.

Sarah Sharp