Unregister driver patch
Hi all!
Here is a patch which implements the unregister_driver() function.
I had to change a field in struct hp_device:
The field "manage" (bool) become "driver" (pointer on a pa_iodc_driver),
since unregister_driver has to know by which driver a device was managed to make unregister proper.unregister_driver() first remove the driver from the list. Then, it looks for devices managed by this driver and changed their "driver" field to NULL.
More over, some drivers call register_driver at least two times, e.g. the gsc parallel port driver. When it append, BUG() is called and a warning is send to give the name of the driver.
NOTE: Since the structure pa_iodc_driver is used to maintain the list of the registered drivers, I think this do not have to be put in data.init section.
Regards,
Matthieu Delahaye
ESIEE Team
http://www.esiee.fr/~puffin
Quoted 1 line · Matthieu Delahaye
Here is a patch which implements the
unregister_driver()function.
Matthieu,
That's excellent!
But no patch was attached. Please post!
thanks,
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
Quoted 8 lines · Grant Grundler
Quoted 1 line · Matthieu Delahaye
Here is a patch which implements the
unregister_driver()function.Matthieu,
That's excellent!But no patch was attached. Please post!
Yes, it's not the first time! The problem was between my keyboard and my chair ;-)
More over, I reposted it before your answer... to myself only... Sorry. I think holidays aren't good for me.
Matthieu
Quoted 4 lines
thanks,
grant
Quoted 8 lines
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/cgi-bin/mailman/listinfo/parisc-linux
arch/parisc/kernel/drivers.c +49 -7
@@ -57,17 +57,23 @@** The list gets built in reverse order...ideally, it shouldn't ** matter but reality will eventually rear it's ugly head. */ +if (driver->next) { +BUG (); +printk (KERN_WARNING "Warning: %s %s was already registered. Skiping.\n" , driver->name,driver->version); +continue ; + } + driver->next = pa_drivers; pa_drivers = driver;for (i=0 ; i < num_devices; i++) { device = &pa_devices[i]; -if (device->managed)continue ; +if ((int )device->driver)continue ;if (0 ==compare_spec (device, driver))continue ;if ( (*driver->callback)(device,driver) ==0 ) { - device->managed=1 ; + device->driver=driver; }else { printk("Warning : device (%d, 0x%x, 0x%x, 0x%x, 0x%x) NOT claimed by %s %s\n", device->hw_type,
@@ -77,6 +83,42 @@ } } } + +return 0 ; +} + + + +int unregister_driver (struct pa_iodc_driver *driver) +{ +int i; +struct hp_device * device; +struct pa_iodc_driver * drv; + +for (;driver->check;driver++) { + drv=pa_drivers; +if (pa_drivers==driver) { + pa_drivers=driver->next; + }else { +while (drv!=NULL && driver!=drv->next) { + drv=drv->next; + } + +if (drv==NULL ) { +printk (KERN_WARNING "unregister_driver: %s %s wasn't registered\n" , driver->name, driver->version); +continue ; + }else { + drv->next=driver->next; + driver->next=NULL ; + } + + } + +for (i=0 ; i < num_devices; i++) { + device = &pa_devices[i]; +if (device->driver==driver) device->driver=NULL ; + } + }return 0 ; }
@@ -127,7 +169,7 @@ d->sversion_rev = iodc_data[4 ]>>4 ; d->opt = iodc_data[7 ]; d->hpa = (void *) hpa; - d->managed =0 ; + d->driver =NULL ; d->reference =parisc_get_reference (d->hw_type, d->hversion, d->sversion);
@@ -145,16 +187,16 @@ {struct pa_iodc_driver *driver = pa_drivers; -while ((0 == hp_dev->managed) && (NULL != driver)) { +while ((NULL == hp_dev->driver) && (NULL != driver)) {if (compare_spec (hp_dev,driver)) - hp_dev->managed = - ((*driver->callback)(hp_dev,driver) ==0 ); + hp_dev->driver = + (((*driver->callback)(hp_dev,driver) ==0 )?driver:NULL ); driver = driver->next; } -return hp_dev->managed; +return (hp_dev->driver==driver); }struct hp_device *get_pa_dev (unsigned int index)
include/asm-parisc/hardware.h +2 -1
@@ -22,7 +22,7 @@unsigned int sversion_rev;struct hp_hardware *reference;/* This is a pointer to the reference */ -unsigned int managed;/* this is if the device has a driver for it */ +struct pa_iodc_driver *driver;/* this is if the device has a driver for it */ unsigned int num_addrs;/* some devices have additional address ranges, */ unsigned long addr[MAX_ADD_ADDRS ];/* which will be stored here */
@@ -113,6 +113,7 @@extern struct hp_device *get_pa_dev (unsigned int index);extern void print_pa_devices (char * buf);extern int register_driver (struct pa_iodc_driver *driver); +extern int unregister_driver (struct pa_iodc_driver *driver);/* inventory.c: */ extern void do_inventory (void );
Quoted 1 line · Matthieu Delahaye
I think holidays aren't good for me.
hmmm...maybe not. ;^)
I'll apply the patch tomorrow barring major objections. The patch looks good to me (with one nit noted below). Thanks!
arch/parisc/kernel/drivers.c line 72
Quoted 2 lines
arch/parisc/kernel/drivers.c +1
-1-if (device->managed)continue ; +if ((int )device->driver)continue ;
I'll change this to:
if (NULL != device->driver) continue;
device->driver is a pointer and should certainly not be cast to an int in this case. For 64-bit, we will loose the upper half of the address and we want to test the entire address. Even if it worked, I wouldn't want to risk it not working in the future.
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
Quoted 5 lines · Grant Grundler
arch/parisc/kernel/drivers.c line 72
Quoted 2 lines
arch/parisc/kernel/drivers.c +1
-1-if (device->managed)continue ; +if ((int )device->driver)continue ;I'll change this to:
if (NULL != device->driver) continue;
we definitely don't want to cast this to an int. But what's wrong with
if (device->driver)
continue;
it reads properly if device has driver then continue'. Sure, it's complex to think about if you try and puzzle it out so, if i've assigned a value to driver then it's not 0, so this is true in which case continue', but if you just read it, it says what it means.
Signature · 1 line
Revolutions do not require corporate support.
Hi Matthieu,
Ok. I lied. One more issue with the patch in register_pa_dev().
Quoted 9 lines · Matthieu Delahaye
arch/parisc/kernel/drivers.c +1
-2@@ -145,16 +187,16 @@ {struct pa_iodc_driver *driver = pa_drivers; -while ((0 == hp_dev->managed) && (NULL != driver)) { +while ((NULL == hp_dev->driver) && (NULL != driver)) {if (compare_spec (hp_dev,driver)) - hp_dev->managed =
arch/parisc/kernel/drivers.c line 200
Quoted 10 lines
arch/parisc/kernel/drivers.c +3
-2- ((*driver->callback)(hp_dev,driver) ==0 ); + hp_dev->driver = + (((*driver->callback)(hp_dev,driver) ==0 )?driv er:NULL); driver = driver->next; } -return hp_dev->managed; +return (hp_dev->driver==driver);
In the case a device is managed, "driver" points to "->next" after we exit the loop and we end up returning false. Right?
I think "return (NULL != hp_dev->driver)" is the correct code.
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
Quoted 22 lines · Grant Grundler
Hi Matthieu,
Ok. I lied. One more issue with the patch inregister_pa_dev().Quoted 17 lines · Matthieu Delahaye
arch/parisc/kernel/drivers.c +3
-3@@ -145,16 +187,16 @@ {struct pa_iodc_driver *driver = pa_drivers; -while ((0 == hp_dev->managed) && (NULL != driver)) { +while ((NULL == hp_dev->driver) && (NULL != driver)) {if (compare_spec (hp_dev,driver)) - hp_dev->managed = - ((*driver->callback)(hp_dev,driver) ==0 ); + hp_dev->driver = + (((*driver->callback)(hp_dev,driver) ==0 )?driv er:NULL); driver = driver->next; }
Quoted 8 lines
arch/parisc/kernel/drivers.c line 199
Quoted 2 lines
arch/parisc/kernel/drivers.c +1
-1-return hp_dev->managed; +return (hp_dev->driver==driver);In the case a device is managed, "driver" points to "->next" after we exit the loop and we end up returning false. Right?
I think "return (NULL !=
hp_dev->driver)" is the correct code.
You're right. I couldn't test this function because I didn't find a way to add devices only after some drivers were already registered.
Matthieu
Quoted 5 lines
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253