Skip to contentSkip to conversation

Unregister driver patch

parisc-linux Matthieu Delahaye 3 participants 7 messages Feb 24, 2001 – Feb 26, 2001 PA-RISC architecture

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw
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

#2 lore raw
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
diff -Nru linux.old/arch/parisc/kernel/drivers.c linux.new/arch/parisc/kernel/drivers.c
@@ -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
diff -Nru linux.old/include/asm-parisc/hardware.h linux.new/include/asm-parisc/hardware.h
@@ -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);
#3 lore raw
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

#4 lore raw
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.
#5 lore raw
In reply to Matthieu Delahaye

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

#6 lore raw
Quoted 22 lines · Grant Grundler

Hi Matthieu,
Ok. I lied. One more issue with the patch in register_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