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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




IMHO the xhci driver is already far too complicated, and this probably just makes
it even worse.

This is an attempt to make it cleaner and less complicated.
In the end this series removes far more more code than it adds:
5 files changed, 373 insertions(+), 530 deletions(-)

The timers are also simplified, so now we only have one timeout timer instead of each command running its own timer.


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)

These solutions require some ring management and possibly expansion code, and increases the complexity again. I selected this simpler approach as I understood that the frequency of commands allocated in ISR is quite low.

This also feels like trying to optimize before we get the main
changes working.


Have you considered adding a 'software command ring' (indexed with the
same value as the hardware one) and using it to hold additional parameters?

It might even be worth only putting a single command into the hardware ring!
That might simplify the timer code.

This is something I haven't thought about. Could be one possibility, but feels like artificially restricting something the HW is designed to care of. The timer code isn't that complex anyway. In addition to init/exit parts we basically got:

queue_command(...)
{
  if (command list is empty)
    /* start timer for new command added to empty list*/
    mod_timer(cmd_timer, timeout);
}

handle_cmd_completion(...)   /* in ISR */
{
  /* A commandcompleted, remove timeout timer */
  del_timer(cmd_timer)

  do_other_stuff

  if(more commands in list)
    mod_timer(cmd_timer, timeout); /* start timer for next command */
}


This still has a fixed constraint on the number of queued commands, but
I suspect that is bounded anyway (a few per device?).

64 commands fit on the command ring segment, I'm not aware of any per-device limits?

Thanks for the comments and ideas

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux