Re: [git pull] fastboot tree for v2.6.28

[Linus Torvalds]

On Fri, 10 Oct 2008, Ingo Molnar wrote:
>
> Please pull the latest fastboot-v28-for-linus git tree from:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git fastboot-v28-for-linus
> 
> the (opt-in) fastboot async bootup feature, as described by Arjan in his 
> v2.6.28 announcement:
> 
>    http://lwn.net/Articles/299591/
> 
> tested and maintained in -tip because tip/tracing embedd-merges this 
> topic. (for the fastboot tracer)

Ok, so I finally looked at the patch, I quite frankly, I think it's 
fundamentally wrong.

This is just an example of the wrongness:

	-module_init(uhci_hcd_init);
	+module_init_async(uhci_hcd_init);
	-module_init(ehci_hcd_init);
	+module_init_async(ehci_hcd_init);
	-module_init(ohci_hcd_mod_init);
	+module_init_async(ohci_hcd_mod_init);

	-device_initcall(pci_init);
	+device_initcall_sync(pci_init);

because there is absolutely _no_ excuse for doing the PCI part of the 
probing asynchronously.

The "ohci_hcd_mod_init" part is _especially_ dangerous, because clearly 
nobody looked at the OHCI setup sequence, which includes a lot of odd 
devices and not all of them at all PCI-related etc. Making it asynchronous 
is not safe, nor is it appropriate.

The fact is, those things all have one thing in common:

 - they call usb_add_hcd, and usb_add_hcd is a horrible and slow piece of 
   crap that doesn't just add the host controller, but does all the 
   probing too.

In other words, what should be fixed is not the initcall sequence, and 
certainly not make PCI device probing (or other random buses) be partly 
asynchronous, but simply make that USB host controller startup function be 
asynchronous.

There are other devices too that may need synchronous core hub discovery 
(eg the actual controller), but that want to do the "devices hanging off 
this controller" asynchronously. Disks come to mind. That shouldn't mean 
that the IDE driver discovery should be asynchronous, it should just mean 
that the driver has some simple way to execute something asynchronously.

In other words, I really think it's very wrong to make that 
"async_init_wq" be somethign that is internal to do_initcalls. It should 
be a workqueue that the initcalls can _choose_ to use at the appropriate 
level (which may be deeper down, like 'usb_add_hcd()'), not be forced to 
use at the outermost one.

You can try to convince me otherwise, but I really do think this patch is 
fundamentally the wrong approach.

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/