Date: Wed, 9 Aug 2006 10:22:01 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: "M. Warner Losh" <imp@bsdimp.com> Cc: freebsd-hackers@freebsd.org Subject: Re: miibus + USB = problem Message-ID: <200608091022.02584.hselasky@c2i.net> In-Reply-To: <20060809.000719.-432838874.imp@bsdimp.com> References: <200608021437.55500.hselasky@c2i.net> <20060809.000719.-432838874.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 09 August 2006 08:07, M. Warner Losh wrote: > In message: <200608021437.55500.hselasky@c2i.net> > > Hans Petter Selasky <hselasky@c2i.net> writes: > : I am currently in the progress of porting "if_aue.c" to my new USB API, > : and have hit a problem that needs to be solved. The problem is that the > : USB system sleeps under "miibus_xxx". Questions are: > : > : Is this allowed? > > Yes. > > : What locks are held when these functions are called ? > > Whatever locks are held when they are called :-). > > miibus is called in the following instances: > > (1) during probe/attach of children (bus_generic_probe is the > typical cause) > (2) During the 'tick' routine. The tick routine is called > from a timeout typically. In the aue driver, for example, > this is done with timeout/untimeout. > > There's been rumblings for a genric mii /dev entry that can be used > for things like kicking off the autonegotiation, etc, but that's not > in the tree now, nor is it likely to be soon. > > The aue driver takes out the AUE_LOCK in these routines, and in > detach, it unregisters the timeout. Alas, it is stupid, and does this > with the lock held, thus ensuring deadlock if the timeout fires after > the lock is acquired, but before the untimeout can complete (meaning > that the timeout routine would sleep forever waiting for the lock, > oops). This is why you can't run the detach routine locked in most > cases. Yes, all of that is gone now. I use "callout_init_mtx()" and that solves the problem, except it does not wait for the last mtx_lock()/mtx_unlock(), in case of a race :-( You need to hold a lock during detach. Else you can risk that the callbacks will re-start functions you have already shut down, like USB transfers, and then you never get detached. > You have to make sure that all other threads have exited, and > that can be tricky. Did you look at the new if_aue, at: http://www.turbocat.net/~hselasky/isdn4bsd/sources/src/sys/dev/usb/ > > : The problem with USB devices, is that the read-register process is very > : slow. It can take up to several milliseconds. And if the device is > : suddenly detached one has to think about adding exit code everywhere. > > Yes. One does. There's no such thing as a free lunch in the kernel, > alas. > > : The solution I see with USB devices is something like this: > : > : if (sc->device_gone) { > : > : exit mutexes ; > : > : kthread_exit(0); > : } > : > : Of course I cannot "kthread_exit()" when the call comes from > : read/write/ioctl, because there is a stack, that expects a returning > : call. If the kernel code was objective C, then maybe one could throw an > : exception or do something alike so that the processor gets out of the > : USB-read procedure. > > Yes. You can't longjmp your way out of this problem. :-) There's no > thread to kill here... > > : Solutions: > : > : 1) use USB hardware polling, not releasing any mutexes, simply using > : DELAY(), until read/writes complete. > > That's insanely ugly. Yes, I agree on that. But if you're out of time, and just need to have something working solid, then just use hardware polling. That's why I provided that option. > > : 2) pre-read all read registers regularly. How can I do this with > : "miibus"? > > You can't do that either. You have to use the aue 'registers' to read > the mii bus management registers. Yes, but I can call "mii_pollstat()" from the "config thread", and then cache the two integers it stores the current status in. > > The only way to deal with this is to have all the places that read the > registers deal with getting an error. If the device really is dying, > you can set a flag so that further reads also return an error to catch > places where the code is sloppy and doesn't check the return value. > This is not going to be a perfect solution. You really need to run all USB reads/writes for a separate thread, that you tear down first. > In fact, the aue_csr_read_* routines do exactly this already, and much > of the code is written to check for the dying. Apart from the > deadlock in the locking unwiding (I'm sure there are others, because > we also call if_detach with this lock held...) I do something similar. --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200608091022.02584.hselasky>