[PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Eric Kerin]

Loading the aic7xxx or aic79xx modules, on a machine that did not
contain the card, left a stale entry in the pci_device list.  This
caused an oops upon loading the next module that registered a
pci_device. 

The patches below will unregister the pci_device if it successfully
registers the device, but does not find any cards. 

Comments are appreciated.


Also, I created a bug in bugzilla.kernel.org for this (before I got in
the mood to fix it myself). What should I do about closing it out?
Attach the patch, or put "posted fix to linux-scsi mailing list"?


Eric Kerin



diff -puN aic7xxx_osm.c aic7xxx_osm.c.oopsfix
--- aic7xxx_osm.c	2004-01-02 03:56:32.606291000 -0500
+++ aic7xxx_osm.c.oopsfix	2004-01-02 03:53:15.236291000 -0500
@@ -844,7 +844,8 @@ ahc_linux_detect(Scsi_Host_Template *tem
 {
 	struct	ahc_softc *ahc;
 	int     found;
-
+	int	pci_reg_stat;
+	
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
 	/*
 	 * It is a bug that the upper layer takes
@@ -891,8 +892,9 @@ ahc_linux_detect(Scsi_Host_Template *tem
 	 */
 	ahc_list_lockinit();
 
+	pci_reg_stat = -1;
 #ifdef CONFIG_PCI
-	ahc_linux_pci_init();
+	pci_reg_stat = ahc_linux_pci_init();
 #endif
 
 #ifdef CONFIG_EISA
@@ -913,7 +915,24 @@ ahc_linux_detect(Scsi_Host_Template *tem
 	spin_lock_irq(&io_request_lock);
 #endif
 	aic7xxx_detect_complete++;
-	return (found);
+	if(found){
+		return (found);
+	}
+
+	/*
+	 * Cleanup everything we registered, 
+	 * because we didn't find any cards.
+	 */
+	
+#ifdef CONFIG_PCI
+	if(pci_reg_stat == 0){
+		ahc_linux_pci_exit();
+	}
+#endif
+#ifdef CONFIG_EISA
+	ahc_linux_eisa_exit();
+#endif
+	return 0;
 }
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)

 



 
diff -puN aic79xx_osm.c aic79xx_osm.c.oopsfix
--- aic79xx_osm.c	2004-01-02 02:46:43.000000000 -0500
+++ aic79xx_osm.c.oopsfix	2004-01-02 03:54:43.846291000 -0500
@@ -856,7 +856,8 @@ ahd_linux_detect(Scsi_Host_Template *tem
 {
 	struct	ahd_softc *ahd;
 	int     found;
-
+	int	pci_reg_stat;
+	
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
 	/*
 	 * It is a bug that the upper layer takes
@@ -906,8 +907,9 @@ ahd_linux_detect(Scsi_Host_Template *tem
 	 */
 	ahd_list_lockinit();
 
+	pci_reg_stat = -1;
 #ifdef CONFIG_PCI
-	ahd_linux_pci_init();
+	pci_reg_stat = ahd_linux_pci_init();
 #endif
 
 	/*
@@ -924,7 +926,20 @@ ahd_linux_detect(Scsi_Host_Template *tem
 	spin_lock_irq(&io_request_lock);
 #endif
 	aic79xx_detect_complete++;
-	return (found);
+	if(found){
+		return (found);
+	}
+
+	/*
+	 * Cleanup everything we registered, 
+	 * because we didn't find any cards.
+	 */
+#ifdef CONFIG_PCI	
+	if(pci_reg_stat == 0){
+		ahd_linux_pci_exit();
+	}
+#endif	
+	return 0;
 }
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Arjan van de Ven]

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

On Fri, 2004-01-02 at 21:50, Eric Kerin wrote:
> Loading the aic7xxx or aic79xx modules, on a machine that did not
> contain the card, left a stale entry in the pci_device list.  This
> caused an oops upon loading the next module that registered a
> pci_device. 
> 
> The patches below will unregister the pci_device if it successfully
> registers the device, but does not find any cards. 
> 
> Comments are appreciated.

well in 2.6 modules are supposed to remain loaded even though no device
is found (since one can be hotplugged later) so that part of the driver
needs changing; other than that yes the driver needs to deregister
itself....

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Eric Kerin]

On Sat, 2004-01-03 at 04:01, Arjan van de Ven wrote: 
> On Fri, 2004-01-02 at 21:50, Eric Kerin wrote:
> > Loading the aic7xxx or aic79xx modules, on a machine that did not
> > contain the card, left a stale entry in the pci_device list.  This
> > caused an oops upon loading the next module that registered a
> > pci_device. 
> > 
> > The patches below will unregister the pci_device if it successfully
> > registers the device, but does not find any cards. 
> > 
> > Comments are appreciated.
> 
> well in 2.6 modules are supposed to remain loaded even though no device
> is found (since one can be hotplugged later) so that part of the driver
> needs changing; other than that yes the driver needs to deregister
> itself....

Yea, I saw examples of both cases (remain loaded, or don't remain
loaded), but there was a comment in include/linux/pci.h that said "For
the module case, a hotplug daemon of some sort should load a module in
response to an insert event."  So I put a little more weight on the ones
that unload being correct.  Oh well live and learn.

Here's a new patch, now it will leave the module loaded if it
successfully registers the pci device.

Eric Kerin



--- aic7xxx_osm.c.original	2004-01-02 03:56:32.000000000 -0500
+++ aic7xxx_osm.c	2004-01-03 05:03:41.000000000 -0500
@@ -844,6 +844,7 @@ ahc_linux_detect(Scsi_Host_Template *tem
 {
 	struct	ahc_softc *ahc;
 	int     found;
+	int	pci_reg_state;
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
 	/*
@@ -891,8 +892,9 @@ ahc_linux_detect(Scsi_Host_Template *tem
 	 */
 	ahc_list_lockinit();
 
+	pci_reg_state = -1;
 #ifdef CONFIG_PCI
-	ahc_linux_pci_init();
+	pci_reg_state = ahc_linux_pci_init();
 #endif
 
 #ifdef CONFIG_EISA
@@ -913,6 +915,10 @@ ahc_linux_detect(Scsi_Host_Template *tem
 	spin_lock_irq(&io_request_lock);
 #endif
 	aic7xxx_detect_complete++;
+	if(pci_reg_state == 0 && found == 0){
+		return(1); 
+	}
+
 	return (found);
 }
 


--- aic79xx_osm.c.original	2004-01-02 02:46:43.000000000 -0500
+++ aic79xx_osm.c	2004-01-03 05:05:52.000000000 -0500
@@ -856,6 +856,7 @@ ahd_linux_detect(Scsi_Host_Template *tem
 {
 	struct	ahd_softc *ahd;
 	int     found;
+	int	pci_reg_state;
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
 	/*
@@ -906,8 +907,9 @@ ahd_linux_detect(Scsi_Host_Template *tem
 	 */
 	ahd_list_lockinit();
 
+	pci_reg_state = -1;
 #ifdef CONFIG_PCI
-	ahd_linux_pci_init();
+	pci_reg_state = ahd_linux_pci_init();
 #endif
 
 	/*
@@ -924,6 +926,10 @@ ahd_linux_detect(Scsi_Host_Template *tem
 	spin_lock_irq(&io_request_lock);
 #endif
 	aic79xx_detect_complete++;
+	if(pci_reg_state == 0 && found == 0){
+		return(1); 
+	}
+
 	return (found);
 }
 

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Jan-Benedict Glaw]

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

On Sat, 2004-01-03 06:12:56 -0500, Eric Kerin <eric@bootseg.com>
wrote in message <1073128375.5478.12.camel@opiate>:
> On Sat, 2004-01-03 at 04:01, Arjan van de Ven wrote: 
> > On Fri, 2004-01-02 at 21:50, Eric Kerin wrote:
> --- aic7xxx_osm.c.original	2004-01-02 03:56:32.000000000 -0500
> +++ aic7xxx_osm.c	2004-01-03 05:03:41.000000000 -0500
> @@ -913,6 +915,10 @@ ahc_linux_detect(Scsi_Host_Template *tem
>  	spin_lock_irq(&io_request_lock);
>  #endif
>  	aic7xxx_detect_complete++;
> +	if(pci_reg_state == 0 && found == 0){
> +		return(1); 
                ^^^^^^^^^^
> +	}
> +
>  	return (found);
        ^^^^^^^^^^^^^^^
>  }

Please, don't. "return" isn't a function, so please omit braces around
the return value. Just remove those braces from the not-touched "return"
statement...

MfG, JBG

-- 
   Jan-Benedict Glaw       jbglaw@lug-owl.de    . +49-172-7608481
   "Eine Freie Meinung in  einem Freien Kopf    | Gegen Zensur | Gegen Krieg
    fuer einen Freien Staat voll Freier Bürger" | im Internet! |   im Irak!
   ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Justin T. Gibbs]

> Loading the aic7xxx or aic79xx modules, on a machine that did not
> contain the card, left a stale entry in the pci_device list.  This
> caused an oops upon loading the next module that registered a
> pci_device. 
> 
> The patches below will unregister the pci_device if it successfully
> registers the device, but does not find any cards. 
> 
> Comments are appreciated.

A more complete solution is to have the generic init function call
the exit function before returning failed status.  This ensures that
*any state*, not just PCI state, is properly cleaned up.

--
Justin

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Justin T. Gibbs]

> well in 2.6 modules are supposed to remain loaded even though no device
> is found (since one can be hotplugged later) so that part of the driver
> needs changing; other than that yes the driver needs to deregister
> itself....

Is this policy change documented somewhere?

--
Justin

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Arjan van de Ven]

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

On Sat, Jan 03, 2004 at 03:24:56PM -0700, Justin T. Gibbs wrote:
> > well in 2.6 modules are supposed to remain loaded even though no device
> > is found (since one can be hotplugged later) so that part of the driver
> > needs changing; other than that yes the driver needs to deregister
> > itself....
> 
> Is this policy change documented somewhere?

Documentation/pci.txt has it (albeit not extremely explicit):

0. Structure of PCI drivers
~~~~~~~~~~~~~~~~~~~~~~~~~~
There exist two kinds of PCI drivers: new-style ones (which leave most of
probing for devices to the PCI layer and support online insertion and
removal of devices [thus supporting PCI, hot-pluggable PCI and CardBus in a
single driver]) <snip>

1. New-style drivers
~~~~~~~~~~~~~~~~~~~
The new-style drivers just call pci_register_driver during their
initialization with a pointer to a structure describing the driver (struct
pci_driver) which contains:
<snip>
        probe           Pointer to a probing function which gets called (during
                        execution of pci_register_driver for already existing
                        devices or later if a new device gets inserted) for all
                        PCI devices which match the ID table and are not handled
                        by the other drivers yet. This function gets passed a
                        pointer to the pci_dev structure representing the device
                        and also which entry in the ID table did the device
                        match. It returns zero when the driver has accepted the
                        device or an error code (negative number) otherwise.
                        This function always gets called from process context,
                        so it can sleep.
<snip the rest>
-----------------

There are 2 reasons to work this way, and I would very much prefer the
adaptec drivers to behave like this:
1) PCI Hotplugging
2) Adding PCI ID's at runtime

While you may not care about 1), a lot of IHV/distros care about 2); eg if
Adaptec puts out a card that can be driven by aic79xx but with just a new PCI ID,
hardware vendors can just add the ID at runtime and have it work in the
(factory) installer etc but only *if the driver is loaded* ....

Greetings,
    Arjan van de Ven



[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Eric Kerin]

Just trying to follow up on this one, make sure this fix dosn't get lost
in the list.

Thanks,
Eric

On Sat, 2004-01-03 at 06:12, Eric Kerin wrote:
> On Sat, 2004-01-03 at 04:01, Arjan van de Ven wrote: 
> > On Fri, 2004-01-02 at 21:50, Eric Kerin wrote:
> > > Loading the aic7xxx or aic79xx modules, on a machine that did not
> > > contain the card, left a stale entry in the pci_device list.  This
> > > caused an oops upon loading the next module that registered a
> > > pci_device. 
> > > 
> > > The patches below will unregister the pci_device if it successfully
> > > registers the device, but does not find any cards. 
> > > 
> > > Comments are appreciated.
> > 
> > well in 2.6 modules are supposed to remain loaded even though no device
> > is found (since one can be hotplugged later) so that part of the driver
> > needs changing; other than that yes the driver needs to deregister
> > itself....
> Here's a new patch, now it will leave the module loaded if it
> successfully registers the pci device.
> 
> Eric Kerin
> 
> 
> 
> --- aic7xxx_osm.c.original	2004-01-02 03:56:32.000000000 -0500
> +++ aic7xxx_osm.c	2004-01-03 05:03:41.000000000 -0500
> @@ -844,6 +844,7 @@ ahc_linux_detect(Scsi_Host_Template *tem
>  {
>  	struct	ahc_softc *ahc;
>  	int     found;
> +	int	pci_reg_state;
>  
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
>  	/*
> @@ -891,8 +892,9 @@ ahc_linux_detect(Scsi_Host_Template *tem
>  	 */
>  	ahc_list_lockinit();
>  
> +	pci_reg_state = -1;
>  #ifdef CONFIG_PCI
> -	ahc_linux_pci_init();
> +	pci_reg_state = ahc_linux_pci_init();
>  #endif
>  
>  #ifdef CONFIG_EISA
> @@ -913,6 +915,10 @@ ahc_linux_detect(Scsi_Host_Template *tem
>  	spin_lock_irq(&io_request_lock);
>  #endif
>  	aic7xxx_detect_complete++;
> +	if(pci_reg_state == 0 && found == 0){
> +		return(1); 
> +	}
> +
>  	return (found);
>  }
>  
> 
> 
> --- aic79xx_osm.c.original	2004-01-02 02:46:43.000000000 -0500
> +++ aic79xx_osm.c	2004-01-03 05:05:52.000000000 -0500
> @@ -856,6 +856,7 @@ ahd_linux_detect(Scsi_Host_Template *tem
>  {
>  	struct	ahd_softc *ahd;
>  	int     found;
> +	int	pci_reg_state;
>  
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
>  	/*
> @@ -906,8 +907,9 @@ ahd_linux_detect(Scsi_Host_Template *tem
>  	 */
>  	ahd_list_lockinit();
>  
> +	pci_reg_state = -1;
>  #ifdef CONFIG_PCI
> -	ahd_linux_pci_init();
> +	pci_reg_state = ahd_linux_pci_init();
>  #endif
>  
>  	/*
> @@ -924,6 +926,10 @@ ahd_linux_detect(Scsi_Host_Template *tem
>  	spin_lock_irq(&io_request_lock);
>  #endif
>  	aic79xx_detect_complete++;
> +	if(pci_reg_state == 0 && found == 0){
> +		return(1); 
> +	}
> +
>  	return (found);
>  }
>  

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Justin T. Gibbs]

> <snip the rest>
> -----------------
> 
> There are 2 reasons to work this way, and I would very much prefer the
> adaptec drivers to behave like this:
> 1) PCI Hotplugging
> 2) Adding PCI ID's at runtime
> 
> While you may not care about 1), a lot of IHV/distros care about 2); eg if
> Adaptec puts out a card that can be driven by aic79xx but with just a new
> PCI ID, hardware vendors can just add the ID at runtime and have it work in
> the (factory) installer etc but only *if the driver is loaded* ....

Well, if you look at the Adaptec drivers, they already attach to all
possible IDs for the chips that are supported.  They do this by registering
a very generic PCI ID table and filtering all requests through their
own tables.  Each of these tables is terminated with a generic entry
that will attach to any of the supported chipsets regardless of the
"fungible bits" in the chip's PCI IDs.  The only exception to this rule is
for chips that are explicitly labeled as owned by a RAID controller or are
in HostRAID mode.  This latter restriction is going away too since
Adaptec is migrating to an MD based solution for its HostRAID products.

In other words, #2 doesn't require these drivers to stay loaded.  For
#1 to work generically, something should load drivers since it isn't
practical to have every driver that could possibly be needed to service
a hot-plug event in loaded in your kernel.

--
Justin

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Arjan van de Ven]

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

On Fri, 2004-01-30 at 00:25, Justin T. Gibbs wrote:
> > <snip the rest>
> > -----------------
> > 
> > There are 2 reasons to work this way, and I would very much prefer the
> > adaptec drivers to behave like this:
> > 1) PCI Hotplugging
> > 2) Adding PCI ID's at runtime
> > 
> > While you may not care about 1), a lot of IHV/distros care about 2); eg if
> > Adaptec puts out a card that can be driven by aic79xx but with just a new
> > PCI ID, hardware vendors can just add the ID at runtime and have it work in
> > the (factory) installer etc but only *if the driver is loaded* ....
> 
> Well, if you look at the Adaptec drivers, they already attach to all
> possible IDs for the chips that are supported.  They do this by registering
> a very generic PCI ID table and filtering all requests through their
> own tables.  

May I ask why this is done? It sounds not the right thing that needs all
kinds of special cases in the OS userland...

Greetings,
    Arjan van de Ven

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] 2.6.0 aic7xxx and aic79xx stale pci_device list entry

[Justin T. Gibbs]

>> Well, if you look at the Adaptec drivers, they already attach to all
>> possible IDs for the chips that are supported.  They do this by registering
>> a very generic PCI ID table and filtering all requests through their
>> own tables.  
> 
> May I ask why this is done?

Because creating a specific table for Linux would duplicate the more generic
probing mechanism that already exists for this purpose in the driver core
which is shared amongst all supported OSes.  Code duplication equals bugs in
my opinion.  This also allows the driver to attach generically to any
chip that it can support.  The Linux probe mechanisms does not allow
this without user intervention (and I wasn't even aware this was possible
until you told me).  I would probably need a 500+ entry table to be able
to attach to all of the possible IDs that an OEM could decide to use when
they put Adaptec Silicon on their MB.

> It sounds not the right thing that needs all
> kinds of special cases in the OS userland...

I don't know why the userland would care.  Just because a driver's
PCI probe routine is called does not mean that it must accept the
device.  I have never encountered any problems with the approach after
three years of using it.

--
Justin