[parisc-linux] Unregister driver patch

[parisc-linux] Unregister driver patch

[Matthieu Delahaye]

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

Re: [parisc-linux] Unregister driver patch

[Grant Grundler]

Matthieu Delahaye wrote:
> 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

Re: [parisc-linux] Unregister driver patch

[Matthieu Delahaye]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

Grant Grundler wrote:

> Matthieu Delahaye wrote:
> > 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

>
> thanks,
> grant
>
> Grant Grundler
> parisc-linux {PCI|IOMMU|SMP} hacker
> +1.408.447.7253
>
> _______________________________________________
> parisc-linux mailing list
> parisc-linux@lists.parisc-linux.org
> http://lists.parisc-linux.org/cgi-bin/mailman/listinfo/parisc-linux

[-- Attachment #2: unregister.diff --]
[-- Type: text/plain, Size: 3526 bytes --]

diff -Nru linux.old/arch/parisc/kernel/drivers.c linux.new/arch/parisc/kernel/drivers.c
--- linux.old/arch/parisc/kernel/drivers.c	Sat Feb 10 18:13:25 2001
+++ linux.new/arch/parisc/kernel/drivers.c	Sat Feb 24 16:40:03 2001
@@ -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)
diff -Nru linux.old/include/asm-parisc/hardware.h linux.new/include/asm-parisc/hardware.h
--- linux.old/include/asm-parisc/hardware.h	Sat Feb 10 18:14:08 2001
+++ linux.new/include/asm-parisc/hardware.h	Sat Feb 24 16:43:00 2001
@@ -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);

Re: [parisc-linux] Unregister driver patch

[Grant Grundler]

Matthieu Delahaye wrote:
> 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!


> -			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

Re: [parisc-linux] Unregister driver patch

[Matthew Wilcox]

On Sun, Feb 25, 2001 at 08:30:55PM -0800, Grant Grundler wrote:
> > -			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.

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] Unregister driver patch

[Grant Grundler]

Hi Matthieu,
Ok. I lied. One more issue with the patch in register_pa_dev().

Matthieu Delahaye wrote:
> @@ -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;
>  	}
>  
> -	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

Re: [parisc-linux] Unregister driver patch

[Matthieu Delahaye]

Grant Grundler wrote:
> 
> Hi Matthieu,
> Ok. I lied. One more issue with the patch in register_pa_dev().
> 
> Matthieu Delahaye wrote:
> > @@ -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;
> >       }
> >
> > -     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

> grant
> 
> Grant Grundler
> parisc-linux {PCI|IOMMU|SMP} hacker
> +1.408.447.7253