On Tue, 16 Jun 2009, Alek Du wrote:
> Have to send v2 version again, fixed a qh_destroy issue.
>
> From bd43fff4f26616f3abe58deffeafac3e05aea4c5 Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@xxxxxxxxx>
> Date: Wed, 10 Jun 2009 11:53:59 +0800
> Subject: [PATCH] EHCI: split ehci_qh structure into hw and sw parts
>
> The ehci_qh structure merged hw and sw together which is not good:
> 1. If HCD has local SRAM, the sw part will consume it too, and it won't
> bring any benefit.
> 2. For non-x86 system, the entire ehci_qh is uncachable, actually we
> only need the hw part to be uncacheable. Split them will give the sw
> part cacheable feature.
I'm sorry to put you through all this work, but the patch you wrote is
backwards!
Instead of letting ehci_qh be the hardware structure and defining a new
ehci_qh_sw to hold the software part, you should have done it the other
way around: Let ehci_qh be the software structure and call the hardware
portion into struct ehci_qh_hw. And add a pointer to the ehci_qh_hw
structure inside ehci_qh. (In fact, you might make it a const pointer
because it's never going to change once it has been assigned.)
Why? Because the way you did it loses all the advantage of using
cacheable memory. For example, you have:
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -429,7 +429,7 @@ static void qh_lines (
> next += temp;
>
> /* hc may be modifying the list as we read it ... */
> - list_for_each (entry, &qh->qtd_list) {
> + list_for_each(entry, &qh->sw->qtd_list) {
Since qtd_list is in the software part, we'd like to get at it without
touching non-cached memory. Instead you now have _two_ dereferences,
where the first uncached!
If you do it as I suggest, this would remain
list_for_each (entry, &qh->qtd_list) {
and the single dereference would be cached.
Alan Stern
--
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
