by Mathias Nyman

[permalink] [raw]

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

On 01/14/2014 02:37 PM, David Laight wrote:
> From: Mathias Nyman
> ...
>>> The fact that you are having to allocate memory ion an ISR ought also to be
>>> ringing alarm bells.
>>
>> It did.
>> Other options are as you said to use a 'software command ring'. Either
>> just pre-allocate a full command ring (64 trbs * sizeof(struct
>> xhci_command)), roughly 2300 bytes when driver loads,
>> or then a smaller pool of commands just used for ISR submitted commands.
>> (We then need to either either expand pool, or start allocating in isr
>> if pool is empty)
>
> If you pre-allocate mapping 1-1 with the command ring entries
> you don't need many of the fields of xhci_command at all.
> So the allocated size will be much smaller.

True, then the struct list_head could be removed.
Even the trb pointer could be removed if the command ring stays in one
segment(just use the trb - base as an index )

...

> Looking at the number of fields in xci_command, why do you need the
> caller to pass a structure at all?
> Just make the fields parameters to the 'send a command' function
> along with a parameter that says whether it can sleep (in kmalloc()
> or if the ring is full depending on the implementation).

Command completion events only tell which trb completed and its status.
The structure is needed to map the trb to the right completion so that
the caller can continue running, check status and move on.

The xhci_command fields forISR allocated commands are really not used
that much. Only status is used if the command timeouts. But we want
these command structures on the command list to make sure eveything else
works (timeout works, commands completion events are in the same order
as commands on our command list etc)

So in a way we're allocating memory in ISR which is not really even used.

I'll try to create something to that avoid that

-Mathias

2014-01-27 23:58:47

by Sarah Sharp

[permalink] [raw]

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

Hi Keith,

You've told me in the past that you've run into an issue where you can
hang the xHCI driver when one of your TeleMetrum boards refuses to
respond to a Set Address command.

Can you test the following patchset, and see if it fixes your problem?
I've applied the patchset against 3.13 here:

https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/log/?h=for-usb-next-command-queue

Thanks,
Sarah Sharp

On Mon, Jan 13, 2014 at 05:05:49PM +0200, Mathias Nyman wrote:
> This is an 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 | 42 ++--
> drivers/usb/host/xhci-mem.c | 22 +-
> drivers/usb/host/xhci-ring.c | 532 ++++++++++++++-----------------------------
> drivers/usb/host/xhci.c | 264 +++++++++++----------
> drivers/usb/host/xhci.h | 43 ++--
> 5 files changed, 373 insertions(+), 530 deletions(-)
>
> --
> 1.8.1.2
>