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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52D5281F.4080108@linux.intel.com>
Date:	Tue, 14 Jan 2014 14:05:51 +0200
From:	Mathias Nyman <mathias.nyman@...ux.intel.com>
To:	David Laight <David.Laight@...LAB.COM>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
CC:	"sarah.a.sharp@...ux.intel.com" <sarah.a.sharp@...ux.intel.com>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 00/10] xhci: re-work command queue management

>
> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/