Ben Collins wrote: > On Wed, Oct 15, 2003 at 10:53:59AM -0700, Andrew Morton wrote: > >>Ben Collins wrote: >> >>>>highlevel_add_host() does read_lock() and then proceeds to do things like >>> >>> > starting kernel threads under that lock. The locking is pretty broken >>> > in there :( >>> >>> No, highlevel_add_host() itself doesn't start any threads. But it does >>> pass around data that needs to be locked from changes, and one of the >>> handlers happens to start a thread, and other things allocate memory >>> (such as this case). >>> >>> It's ugly, and I've been trying to clean it up. This case can be fixed >>> quickly with a simple check in hpsb_create_hostinfo() to pass GFP_ATOMIC >>> to kmalloc. >> >>nodemgr_add_host() looks like the hard one. Maybe make hl_drivers_lock a >>sleeping lock? > > > Problem is, things like bus resets happen in interrupt, and while I can > push off some things to occur in the nodemgr thread, a lot of other > stuff has to happen in the interrupt, and they require the same lock. I was looking briefly at this too, and as you say, the problem is that some things have to happen in interrupt, others happen in process context. I've attached a patch that implements one way to fix it: double book-keeping - we maintain two lists of the highlevel drivers, one protected by a semaphore another protected by the rw spinlock. The lists are identical, except between the two list_add_tail()'s (and the two list_del()'s), but that doesn't allow any harmful race conditions. A more radical approach would be to split the highlevel interface into two interfaces add_host() + remove_host() in a hpsb_host_notification interface and the rest in another interface. The driver would have to register both interfaces if it needs them. Some drivers only use add_host() and remove_host(), so they could register only the hpsb_host_notification interface. best regards, Kristian