From 3833f137f9ebe3b9cfbfb026c53c5726d04cb485 Mon Sep 17 00:00:00 2001 From: Alexander Viro Date: Wed, 4 Feb 2004 16:58:49 -0800 Subject: [PATCH] [PATCH] parport fixes (2/6) We use a new mutex to protect all additions/removals of drivers and ports. That cures a lot of insanity: * driver removals can't hit us in the middle of attach_driver_chain(). Old code simply dies on that. * port removals can't hit us in the middle of driver registration. Again, old code dies on that. * driver ->detach() is allowed to block now. * we are guaranteed that by the time when parport_unregister_driver() returns, all ->detach() calls are finished. Old code did _not_ guarantee that (read: was inherently racy since rmmod of driver could race with port removal and get driver->detach(port) called after the module was gone). * we are guaranteed that driver->attach(port) won't be called more than once. With the old code that was a matter of luck. * removed piles and piles of braindead code. --- drivers/parport/share.c | 129 +++++++++++----------------------------- 1 file changed, 36 insertions(+), 93 deletions(-) diff --git a/drivers/parport/share.c b/drivers/parport/share.c index ccb4f5447cc4d..ae02714fb723f 100644 --- a/drivers/parport/share.c +++ b/drivers/parport/share.c @@ -49,7 +49,8 @@ static LIST_HEAD(all_ports); static spinlock_t full_list_lock = SPIN_LOCK_UNLOCKED; static struct parport_driver *driver_chain = NULL; -static spinlock_t driverlist_lock = SPIN_LOCK_UNLOCKED; + +static DECLARE_MUTEX(registration_lock); /* What you can do to a port that's gone away.. */ static void dead_write_lines (struct parport *p, unsigned char b){} @@ -102,55 +103,19 @@ static struct parport_operations dead_ops = { /* Call attach(port) for each registered driver. */ static void attach_driver_chain(struct parport *port) { + /* caller has exclusive registration_lock */ struct parport_driver *drv; - void (**attach) (struct parport *); - int count = 0, i; - - /* This is complicated because attach() must be able to block, - * but we can't let it do that while we're holding a - * spinlock. */ - - spin_lock (&driverlist_lock); for (drv = driver_chain; drv; drv = drv->next) - count++; - spin_unlock (&driverlist_lock); - - /* Drivers can unregister here; that's okay. If they register - * they'll be given an attach during parport_register_driver, - * so that's okay too. The only worry is that someone might - * get given an attach twice if they registered just before - * this function gets called. */ - - /* Hmm, this could be fixed with a generation number.. - * FIXME */ - - attach = kmalloc (sizeof (void(*)(struct parport *)) * count, - GFP_KERNEL); - if (!attach) { - printk (KERN_WARNING "parport: not enough memory to attach\n"); - return; - } - - spin_lock (&driverlist_lock); - for (i = 0, drv = driver_chain; drv && i < count; drv = drv->next) - attach[i++] = drv->attach; - spin_unlock (&driverlist_lock); - - for (count = 0; count < i; count++) - (*attach[count]) (port); - - kfree (attach); + drv->attach(port); } /* Call detach(port) for each registered driver. */ static void detach_driver_chain(struct parport *port) { + /* caller has exclusive registration_lock */ struct parport_driver *drv; - - spin_lock (&driverlist_lock); for (drv = driver_chain; drv; drv = drv->next) drv->detach (port); - spin_unlock (&driverlist_lock); } /* Ask kmod for some lowlevel drivers. */ @@ -178,7 +143,7 @@ static void get_lowlevel_driver (void) * pointer it must call parport_get_port() to do so. Calling * parport_register_device() on that port will do this for you. * - * The driver's detach() function may not block. The port that + * The driver's detach() function may block. The port that * detach() is given will be valid for the duration of the * callback, but if the driver wants to take a copy of the * pointer it must call parport_get_port() to do so. @@ -189,8 +154,6 @@ static void get_lowlevel_driver (void) int parport_register_driver (struct parport_driver *drv) { struct parport *port; - struct parport **ports; - int count = 0, i; if (!portlist) get_lowlevel_driver (); @@ -203,31 +166,12 @@ int parport_register_driver (struct parport_driver *drv) * it. But we need to hold a spinlock to iterate over the * list of ports.. */ - spin_lock (&parportlist_lock); + down(®istration_lock); for (port = portlist; port; port = port->next) - count++; - spin_unlock (&parportlist_lock); - - ports = kmalloc (sizeof (struct parport *) * count, GFP_KERNEL); - if (!ports) - printk (KERN_WARNING "parport: not enough memory to attach\n"); - else { - spin_lock (&parportlist_lock); - for (i = 0, port = portlist; port && i < count; - port = port->next) - ports[i++] = port; - spin_unlock (&parportlist_lock); - - for (count = 0; count < i; count++) - drv->attach (ports[count]); - - kfree (ports); - } - - spin_lock (&driverlist_lock); + drv->attach(port); drv->next = driver_chain; driver_chain = drv; - spin_unlock (&driverlist_lock); + up(®istration_lock); return 0; } @@ -245,44 +189,38 @@ int parport_register_driver (struct parport_driver *drv) * be called, and for each port that attach() was called for, the * detach() routine will have been called. * - * If the caller's attach() function can block, it is their - * responsibility to make sure to wait for it to exit before - * unloading. - * - * All the driver's detach() calls are guaranteed to have + * All the driver's attach() and detach() calls are guaranteed to have * finished by the time this function returns. - * - * The driver's detach() call is not allowed to block. **/ void parport_unregister_driver (struct parport_driver *arg) { - struct parport_driver *drv = driver_chain, *olddrv = NULL; + struct parport_driver *drv, *olddrv = NULL; + down(®istration_lock); + drv = driver_chain; while (drv) { if (drv == arg) { struct parport *port; - spin_lock (&driverlist_lock); if (olddrv) olddrv->next = drv->next; else driver_chain = drv->next; - spin_unlock (&driverlist_lock); /* Call the driver's detach routine for each * port to clean up any resources that the * attach routine acquired. */ - spin_lock (&parportlist_lock); for (port = portlist; port; port = port->next) drv->detach (port); - spin_unlock (&parportlist_lock); + up(®istration_lock); return; } olddrv = drv; drv = drv->next; } + up(®istration_lock); } static void free_port (struct parport *port) @@ -476,22 +414,6 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma, void parport_announce_port (struct parport *port) { - /* We are locked against anyone else performing alterations, but - * because of parport_enumerate people can still _read_ the list - * while we are changing it; so be careful.. - * - * It's okay to have portlist_tail a little bit out of sync - * since it's only used for changing the list, not for reading - * from it. - */ - - spin_lock_irq(&parportlist_lock); - if (portlist_tail) - portlist_tail->next = port; - portlist_tail = port; - if (!portlist) - portlist = port; - spin_unlock_irq(&parportlist_lock); #ifdef CONFIG_PARPORT_1284 /* Analyse the IEEE1284.3 topology of the port. */ if (parport_daisy_init (port) == 0) { @@ -509,8 +431,27 @@ void parport_announce_port (struct parport *port) } #endif + down(®istration_lock); + /* We are locked against anyone else performing alterations, but + * because of parport_enumerate people can still _read_ the list + * while we are changing it; so be careful.. + * + * It's okay to have portlist_tail a little bit out of sync + * since it's only used for changing the list, not for reading + * from it. + */ + + spin_lock_irq(&parportlist_lock); + if (portlist_tail) + portlist_tail->next = port; + portlist_tail = port; + if (!portlist) + portlist = port; + spin_unlock_irq(&parportlist_lock); + /* Let drivers know that a new port has arrived. */ attach_driver_chain (port); + up(®istration_lock); } /** @@ -536,6 +477,7 @@ void parport_unregister_port(struct parport *port) { struct parport *p; + down(®istration_lock); port->ops = &dead_ops; /* Spread the word. */ @@ -565,6 +507,7 @@ void parport_unregister_port(struct parport *port) "%s not found in port list!\n", port->name); } spin_unlock(&parportlist_lock); + up(®istration_lock); /* Yes, parport_enumerate _is_ unsafe. Don't use it. */ parport_put_port (port); -- 2.53.0