[PATCH] release_pcibus_dev() crash

[PATCH] release_pcibus_dev() crash

[John Rose]

During the course of a hotplug removal of a PCI bus, release_pcibus_dev()
attempts to remove attribute files from a kobject directory that no longer
exists.  The calls that cause the crash were recently added, and this patch
removes them.  The partial oops traceback, observed on ppc64, is:

[c0000000ba1eb2d0] c0000000000f6eb4 .sysfs_remove_file+0x20/0x38
[c0000000ba1eb350] c00000000024c564 .class_device_remove_file+0x28/0x40
[c0000000ba1eb3d0] c0000000001dad5c .release_pcibus_dev+0x38/0x90
[c0000000ba1eb460] c00000000024c884 .class_dev_release+0x50/0x84
[c0000000ba1eb4e0] c0000000001cf4a8 .kobject_cleanup+0xec/0xf4
[c0000000ba1eb580] c0000000001d02f8 .kref_put+0x90/0x98
[c0000000ba1eb600] c0000000001cf500 .kobject_put+0x34/0x50
[c0000000ba1eb680] c00000000024d2ac .class_device_put+0x1c/0x34
[c0000000ba1eb700] c00000000024d194 .class_device_unregister+0x28/0x44
[c0000000ba1eb790] c0000000001dc8d8 .pci_remove_bus+0x78/0x98
...

The removal of the class device from sysfs is carried out explicitly by
class_device_del(), which occurs prior to class_device_put().  The class device
is gone from sysfs by the time class_device_put() is called.  As such, this
release function should not carry out sysfs cleanups for the class device.

I'm unsure how pci_remove_legacy_files() doesn't cause the same crash for those
who implemented it, but I'll leave that alone for now.

Thanks-
John

Signed-off-by: John Rose <johnrose@austin.ibm.com>

diff -puN drivers/pci/probe.c~01_release_pcibus_dev drivers/pci/probe.c
--- 2_6_linus_2/drivers/pci/probe.c~01_release_pcibus_dev	2005-01-12 18:22:00.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/probe.c	2005-01-12 18:22:29.000000000 -0600
@@ -96,9 +96,6 @@ static void release_pcibus_dev(struct cl
 	struct pci_bus *pci_bus = to_pci_bus(class_dev);
 
 	pci_remove_legacy_files(pci_bus);
-	class_device_remove_file(&pci_bus->class_dev,
-				 &class_device_attr_cpuaffinity);
-	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
 	if (pci_bus->bridge)
 		put_device(pci_bus->bridge);
 	kfree(pci_bus);

_

Re: [PATCH] release_pcibus_dev() crash

[Jesse Barnes]

On Wednesday, January 12, 2005 4:39 pm, John Rose wrote:
> The removal of the class device from sysfs is carried out explicitly by
> class_device_del(), which occurs prior to class_device_put().  The class
> device is gone from sysfs by the time class_device_put() is called.  As
> such, this release function should not carry out sysfs cleanups for the
> class device.
>
> I'm unsure how pci_remove_legacy_files() doesn't cause the same crash for
> those who implemented it, but I'll leave that alone for now.

Feel free to fix it too.  I haven't tested the removal case, so thanks for 
catching it.

Jesse

Re: [PATCH] release_pcibus_dev() crash

[John Rose]

Jesse-

I'm having trouble with enabling the legacy PCI stuff.  I added #define
HAVE_PCI_LEGACY to pci.h, but probe.c doesn't build:

drivers/pci/probe.c: In function `pci_create_legacy_files':
drivers/pci/probe.c:50: error: `pci_read_legacy_io' undeclared (first
use in this function)
drivers/pci/probe.c:50: error: (Each undeclared identifier is reported
only oncedrivers/pci/probe.c:50: error: for each function it appears
in.)
drivers/pci/probe.c:51: error: `pci_write_legacy_io' undeclared (first
use in this function)
drivers/pci/probe.c:60: error: `pci_mmap_legacy_mem' undeclared (first
use in this function)

Am I missing something obvious? :)

Thanks-
John

On Wed, 2005-01-12 at 18:55, Jesse Barnes wrote:
> On Wednesday, January 12, 2005 4:39 pm, John Rose wrote:
> > The removal of the class device from sysfs is carried out explicitly by
> > class_device_del(), which occurs prior to class_device_put().  The class
> > device is gone from sysfs by the time class_device_put() is called.  As
> > such, this release function should not carry out sysfs cleanups for the
> > class device.
> >
> > I'm unsure how pci_remove_legacy_files() doesn't cause the same crash for
> > those who implemented it, but I'll leave that alone for now.
> 
> Feel free to fix it too.  I haven't tested the removal case, so thanks for 
> catching it.
> 
> Jesse
> 

Re: [PATCH] release_pcibus_dev() crash

[Jesse Barnes]

On Thursday, January 13, 2005 9:11 am, John Rose wrote:
> Jesse-
>
> I'm having trouble with enabling the legacy PCI stuff.  I added #define
> HAVE_PCI_LEGACY to pci.h, but probe.c doesn't build:
>
> drivers/pci/probe.c: In function `pci_create_legacy_files':
> drivers/pci/probe.c:50: error: `pci_read_legacy_io' undeclared (first
> use in this function)
> drivers/pci/probe.c:50: error: (Each undeclared identifier is reported
> only oncedrivers/pci/probe.c:50: error: for each function it appears
> in.)
> drivers/pci/probe.c:51: error: `pci_write_legacy_io' undeclared (first
> use in this function)
> drivers/pci/probe.c:60: error: `pci_mmap_legacy_mem' undeclared (first
> use in this function)
>
> Am I missing something obvious? :)

Maybe, did you read Documentation/filesystems/sysfs-pci.c?  You need to do 
more than just enable HAVE_PCI_LEGACY, you also need to implement some 
functions.

Jesse

Re: [PATCH] release_pcibus_dev() crash

[John Rose]

> Maybe, did you read Documentation/filesystems/sysfs-pci.c?  You need to do 
> more than just enable HAVE_PCI_LEGACY, you also need to implement some 
> functions.

This sounds like more than I bargained for.  I'll leave the patch as-is,
since I don't currently have the means to test a fix for the legacy IO
stuff.  Also because it doesn't crash on my architecture :)

If you get some time, my suggestion is to scrap
pci_remove_legacy_files(), and free the pci_bus->legacy_io field in
pci_remove_bus().  The binary sysfs files will be cleaned up
automatically as the class device is deleted, as described in the
parent.

Thanks-
John

Re: [PATCH] release_pcibus_dev() crash

[Greg KH]

On Thu, Jan 13, 2005 at 11:49:11AM -0600, John Rose wrote:
> > Maybe, did you read Documentation/filesystems/sysfs-pci.c?  You need to do 
> > more than just enable HAVE_PCI_LEGACY, you also need to implement some 
> > functions.
> 
> This sounds like more than I bargained for.  I'll leave the patch as-is,
> since I don't currently have the means to test a fix for the legacy IO
> stuff.  Also because it doesn't crash on my architecture :)
> 
> If you get some time, my suggestion is to scrap
> pci_remove_legacy_files(), and free the pci_bus->legacy_io field in
> pci_remove_bus().  The binary sysfs files will be cleaned up
> automatically as the class device is deleted, as described in the
> parent.

No, don't rely on this please.  Explicitly clean up the files, it's
nicer that way, and when sysfs changes to not clean them up for you, it
will be less changes then.

thanks,

greg k-h

Re: [PATCH] release_pcibus_dev() crash

[Jesse Barnes]

On Thursday, January 13, 2005 10:18 am, Greg KH wrote:
> On Thu, Jan 13, 2005 at 11:49:11AM -0600, John Rose wrote:
> > > Maybe, did you read Documentation/filesystems/sysfs-pci.c?  You need to
> > > do more than just enable HAVE_PCI_LEGACY, you also need to implement
> > > some functions.
> >
> > This sounds like more than I bargained for.  I'll leave the patch as-is,
> > since I don't currently have the means to test a fix for the legacy IO
> > stuff.  Also because it doesn't crash on my architecture :)
> >
> > If you get some time, my suggestion is to scrap
> > pci_remove_legacy_files(), and free the pci_bus->legacy_io field in
> > pci_remove_bus().  The binary sysfs files will be cleaned up
> > automatically as the class device is deleted, as described in the
> > parent.
>
> No, don't rely on this please.  Explicitly clean up the files, it's
> nicer that way, and when sysfs changes to not clean them up for you, it
> will be less changes then.

So does the crash indicate that something is removing the directory, 
containing the attributes to be removed, before it should?  How should the 
oops be fixed?

Thanks,
Jesse

Re: [PATCH] release_pcibus_dev() crash

[Greg KH]

On Thu, Jan 13, 2005 at 10:21:19AM -0800, Jesse Barnes wrote:
> On Thursday, January 13, 2005 10:18 am, Greg KH wrote:
> > On Thu, Jan 13, 2005 at 11:49:11AM -0600, John Rose wrote:
> > > > Maybe, did you read Documentation/filesystems/sysfs-pci.c?  You need to
> > > > do more than just enable HAVE_PCI_LEGACY, you also need to implement
> > > > some functions.
> > >
> > > This sounds like more than I bargained for.  I'll leave the patch as-is,
> > > since I don't currently have the means to test a fix for the legacy IO
> > > stuff.  Also because it doesn't crash on my architecture :)
> > >
> > > If you get some time, my suggestion is to scrap
> > > pci_remove_legacy_files(), and free the pci_bus->legacy_io field in
> > > pci_remove_bus().  The binary sysfs files will be cleaned up
> > > automatically as the class device is deleted, as described in the
> > > parent.
> >
> > No, don't rely on this please.  Explicitly clean up the files, it's
> > nicer that way, and when sysfs changes to not clean them up for you, it
> > will be less changes then.
> 
> So does the crash indicate that something is removing the directory, 
> containing the attributes to be removed, before it should?  How should the 
> oops be fixed?

Yes, this should be fixed, as it is incorrect, sorry in not seeing it
before.  Those files should be removed in the pci_remove_bus() function,
before the class_device_unregister() call.

So, does the patch below fix the problem for you?
(warning, not even compile tested)

thanks,

greg k-h

===== probe.c 1.91 vs edited =====
--- 1.91/drivers/pci/probe.c	2005-01-11 09:14:53 -08:00
+++ edited/probe.c	2005-01-13 10:34:43 -08:00
@@ -95,10 +95,6 @@ static void release_pcibus_dev(struct cl
 {
 	struct pci_bus *pci_bus = to_pci_bus(class_dev);
 
-	pci_remove_legacy_files(pci_bus);
-	class_device_remove_file(&pci_bus->class_dev,
-				 &class_device_attr_cpuaffinity);
-	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
 	if (pci_bus->bridge)
 		put_device(pci_bus->bridge);
 	kfree(pci_bus);
===== remove.c 1.10 vs edited =====
--- 1.10/drivers/pci/remove.c	2004-11-15 09:27:14 -08:00
+++ edited/remove.c	2005-01-13 10:36:23 -08:00
@@ -61,15 +61,19 @@ int pci_remove_device_safe(struct pci_de
 }
 EXPORT_SYMBOL(pci_remove_device_safe);
 
-void pci_remove_bus(struct pci_bus *b)
+void pci_remove_bus(struct pci_bus *pci_bus)
 {
-	pci_proc_detach_bus(b);
+	pci_proc_detach_bus(pci_bus);
 
 	spin_lock(&pci_bus_lock);
-	list_del(&b->node);
+	list_del(&pci_bus->node);
 	spin_unlock(&pci_bus_lock);
 
-	class_device_unregister(&b->class_dev);
+	pci_remove_legacy_files(pci_bus);
+	class_device_remove_file(&pci_bus->class_dev,
+				 &class_device_attr_cpuaffinity);
+	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
+	class_device_unregister(&pci_bus->class_dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 

Re: [PATCH] release_pcibus_dev() crash

[John Rose]

> So, does the patch below fix the problem for you?
> (warning, not even compile tested)

Unfortunately, the class device attribute and pci_remove_legacy_files()
are statically defined in probe.c.  The patch below augments yours to account
for this.

This set of changes also fixes the crash.  I'll let you decide if explicitly
cleaning up these files is worth exporting this stuff outside of probe.c.  
Also, suggestions are welcome for a cleaner way to do this, I'm not totally
comfortable with the ifdef's and inlines :)

Thanks-
John

Signed-off-by: John Rose <johnrose@austin.ibm.com>

diff -puN drivers/pci/probe.c~01_release_pcibus_dev drivers/pci/probe.c
--- 2_6_linus_2/drivers/pci/probe.c~01_release_pcibus_dev	2005-01-13 13:41:08.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/probe.c	2005-01-13 13:57:27.000000000 -0600
@@ -70,7 +70,7 @@ static void pci_remove_legacy_files(stru
 }
 #else /* !HAVE_PCI_LEGACY */
 static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
-static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
+inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
 #endif /* HAVE_PCI_LEGACY */
 
 /*
@@ -86,7 +86,7 @@ static ssize_t pci_bus_show_cpuaffinity(
 		buf[ret++] = '\n';
 	return ret;
 }
-static CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
+CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
 
 /*
  * PCI Bus Class
@@ -95,10 +95,6 @@ static void release_pcibus_dev(struct cl
 {
 	struct pci_bus *pci_bus = to_pci_bus(class_dev);
 
-	pci_remove_legacy_files(pci_bus);
-	class_device_remove_file(&pci_bus->class_dev,
-				 &class_device_attr_cpuaffinity);
-	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
 	if (pci_bus->bridge)
 		put_device(pci_bus->bridge);
 	kfree(pci_bus);
diff -puN drivers/pci/remove.c~01_release_pcibus_dev drivers/pci/remove.c
--- 2_6_linus_2/drivers/pci/remove.c~01_release_pcibus_dev	2005-01-13 13:42:39.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/remove.c	2005-01-13 13:47:18.000000000 -0600
@@ -61,15 +61,18 @@ int pci_remove_device_safe(struct pci_de
 }
 EXPORT_SYMBOL(pci_remove_device_safe);
 
-void pci_remove_bus(struct pci_bus *b)
+void pci_remove_bus(struct pci_bus *pci_bus)
 {
-	pci_proc_detach_bus(b);
+	pci_proc_detach_bus(pci_bus);
 
 	spin_lock(&pci_bus_lock);
-	list_del(&b->node);
+	list_del(&pci_bus->node);
 	spin_unlock(&pci_bus_lock);
-
-	class_device_unregister(&b->class_dev);
+	pci_remove_legacy_files(pci_bus);
+	class_device_remove_file(&pci_bus->class_dev,
+		&class_device_attr_cpuaffinity);
+	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
+	class_device_unregister(&pci_bus->class_dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
diff -puN include/linux/pci.h~01_release_pcibus_dev include/linux/pci.h
--- 2_6_linus_2/include/linux/pci.h~01_release_pcibus_dev	2005-01-13 13:53:28.000000000 -0600
+++ 2_6_linus_2-johnrose/include/linux/pci.h	2005-01-13 13:58:06.000000000 -0600
@@ -1055,8 +1055,10 @@ enum pci_fixup_pass {
 
 
 void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev);
+void pci_remove_legacy_files(struct pci_bus *bus);
 
 extern int pci_pci_problems;
+extern struct class_device_attribute class_device_attr_cpuaffinity;
 #define PCIPCI_FAIL		1
 #define PCIPCI_TRITON		2
 #define PCIPCI_NATOMA		4

_

Re: [PATCH] release_pcibus_dev() crash

[Greg KH]

On Thu, Jan 13, 2005 at 02:12:15PM -0600, John Rose wrote:
> > So, does the patch below fix the problem for you?
> > (warning, not even compile tested)
> 
> Unfortunately, the class device attribute and pci_remove_legacy_files()
> are statically defined in probe.c.  The patch below augments yours to account
> for this.
> 
> This set of changes also fixes the crash.  I'll let you decide if explicitly
> cleaning up these files is worth exporting this stuff outside of probe.c.  
> Also, suggestions are welcome for a cleaner way to do this, I'm not totally
> comfortable with the ifdef's and inlines :)

Yeah, I don't think some of it is correct :)

> @@ -70,7 +70,7 @@ static void pci_remove_legacy_files(stru
>  }
>  #else /* !HAVE_PCI_LEGACY */
>  static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
> -static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
> +inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
>  #endif /* HAVE_PCI_LEGACY */

Why make it inline?  I don't think that will work with gcc 4.

> -static CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
> +CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);

That's fine with me.

>  /*
>   * PCI Bus Class
> @@ -95,10 +95,6 @@ static void release_pcibus_dev(struct cl
>  {
>  	struct pci_bus *pci_bus = to_pci_bus(class_dev);
>  
> -	pci_remove_legacy_files(pci_bus);
> -	class_device_remove_file(&pci_bus->class_dev,
> -				 &class_device_attr_cpuaffinity);
> -	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
>  	if (pci_bus->bridge)
>  		put_device(pci_bus->bridge);
>  	kfree(pci_bus);
> diff -puN drivers/pci/remove.c~01_release_pcibus_dev drivers/pci/remove.c
> --- 2_6_linus_2/drivers/pci/remove.c~01_release_pcibus_dev	2005-01-13 13:42:39.000000000 -0600
> +++ 2_6_linus_2-johnrose/drivers/pci/remove.c	2005-01-13 13:47:18.000000000 -0600
> @@ -61,15 +61,18 @@ int pci_remove_device_safe(struct pci_de
>  }
>  EXPORT_SYMBOL(pci_remove_device_safe);
>  
> -void pci_remove_bus(struct pci_bus *b)
> +void pci_remove_bus(struct pci_bus *pci_bus)
>  {
> -	pci_proc_detach_bus(b);
> +	pci_proc_detach_bus(pci_bus);
>  
>  	spin_lock(&pci_bus_lock);
> -	list_del(&b->node);
> +	list_del(&pci_bus->node);
>  	spin_unlock(&pci_bus_lock);
> -
> -	class_device_unregister(&b->class_dev);
> +	pci_remove_legacy_files(pci_bus);
> +	class_device_remove_file(&pci_bus->class_dev,
> +		&class_device_attr_cpuaffinity);
> +	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
> +	class_device_unregister(&pci_bus->class_dev);
>  }
>  EXPORT_SYMBOL(pci_remove_bus);
>  
> diff -puN include/linux/pci.h~01_release_pcibus_dev include/linux/pci.h
> --- 2_6_linus_2/include/linux/pci.h~01_release_pcibus_dev	2005-01-13 13:53:28.000000000 -0600
> +++ 2_6_linus_2-johnrose/include/linux/pci.h	2005-01-13 13:58:06.000000000 -0600

No, put these in drivers/pci/pci.h instead.

Care to redo this?

thanks,

greg k-h

Re: [PATCH] release_pcibus_dev() crash

[John Rose]

> Care to redo this?

Good points.  How's this:

Signed-off-by: John Rose <johnrose@austin.ibm.com>

diff -puN drivers/pci/probe.c~01_release_pcibus_dev drivers/pci/probe.c
--- 2_6_linus_2/drivers/pci/probe.c~01_release_pcibus_dev	2005-01-13 14:49:21.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/probe.c	2005-01-13 14:49:21.000000000 -0600
@@ -70,7 +70,7 @@ static void pci_remove_legacy_files(stru
 }
 #else /* !HAVE_PCI_LEGACY */
 static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
-static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
+void pci_remove_legacy_files(struct pci_bus *bus) { return; }
 #endif /* HAVE_PCI_LEGACY */
 
 /*
@@ -86,7 +86,7 @@ static ssize_t pci_bus_show_cpuaffinity(
 		buf[ret++] = '\n';
 	return ret;
 }
-static CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
+CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
 
 /*
  * PCI Bus Class
@@ -95,10 +95,6 @@ static void release_pcibus_dev(struct cl
 {
 	struct pci_bus *pci_bus = to_pci_bus(class_dev);
 
-	pci_remove_legacy_files(pci_bus);
-	class_device_remove_file(&pci_bus->class_dev,
-				 &class_device_attr_cpuaffinity);
-	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
 	if (pci_bus->bridge)
 		put_device(pci_bus->bridge);
 	kfree(pci_bus);
diff -puN drivers/pci/remove.c~01_release_pcibus_dev drivers/pci/remove.c
--- 2_6_linus_2/drivers/pci/remove.c~01_release_pcibus_dev	2005-01-13 14:49:21.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/remove.c	2005-01-13 14:49:21.000000000 -0600
@@ -61,15 +61,18 @@ int pci_remove_device_safe(struct pci_de
 }
 EXPORT_SYMBOL(pci_remove_device_safe);
 
-void pci_remove_bus(struct pci_bus *b)
+void pci_remove_bus(struct pci_bus *pci_bus)
 {
-	pci_proc_detach_bus(b);
+	pci_proc_detach_bus(pci_bus);
 
 	spin_lock(&pci_bus_lock);
-	list_del(&b->node);
+	list_del(&pci_bus->node);
 	spin_unlock(&pci_bus_lock);
-
-	class_device_unregister(&b->class_dev);
+	pci_remove_legacy_files(pci_bus);
+	class_device_remove_file(&pci_bus->class_dev,
+		&class_device_attr_cpuaffinity);
+	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
+	class_device_unregister(&pci_bus->class_dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
diff -puN drivers/pci/pci.h~01_release_pcibus_dev drivers/pci/pci.h
--- 2_6_linus_2/drivers/pci/pci.h~01_release_pcibus_dev	2005-01-13 14:49:21.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/pci.h	2005-01-13 14:50:06.000000000 -0600
@@ -59,12 +59,14 @@ struct pci_visit {
 extern int pci_visit_dev(struct pci_visit *fn,
 			 struct pci_dev_wrapped *wrapped_dev,
 			 struct pci_bus_wrapped *wrapped_parent);
+extern void pci_remove_legacy_files(struct pci_bus *bus);
 
 /* Lock for read/write access to pci device and bus lists */
 extern spinlock_t pci_bus_lock;
 
 extern int pcie_mch_quirk;
 extern struct device_attribute pci_dev_attrs[];
+extern struct class_device_attribute class_device_attr_cpuaffinity;
 
 /**
  * pci_match_one_device - Tell if a PCI device structure has a matching

_

Re: [PATCH] release_pcibus_dev() crash

[Greg KH]

On Thu, Jan 13, 2005 at 02:54:39PM -0600, John Rose wrote:
> > Care to redo this?
> 
> Good points.  How's this:

Closer (you forgot a changelog entry too...)

>  }
>  #else /* !HAVE_PCI_LEGACY */
>  static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
> -static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
> +void pci_remove_legacy_files(struct pci_bus *bus) { return; }
>  #endif /* HAVE_PCI_LEGACY */

What about the HAVE_PCI_LEGACY version of the file?

thanks,

greg k-h

Re: [PATCH] release_pcibus_dev() crash

[John Rose]

> Closer (you forgot a changelog entry too...)

Just to be a brat, I'll point out that I couldn't find a single user of
CLASS_DEVICE_ATTR that explicitly cleans things up like we're doing here.  That
would include firmware and net-sysfs stuff.  Maybe enforcing such a policy upon
device removal would increase participation :)  But okay, here's another try:

During the course of a hotplug removal of a PCI bus, release_pcibus_dev()
attempts to remove attribute files from a kobject directory that no longer
exists.  This patch moves these calls to pci_remove_bus(), where they can work
as intended.

Signed-off-by: John Rose <johnrose@austin.ibm.com>

diff -puN drivers/pci/probe.c~01_release_pcibus_dev drivers/pci/probe.c
--- 2_6_linus_2/drivers/pci/probe.c~01_release_pcibus_dev	2005-01-13 14:49:21.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/probe.c	2005-01-13 15:09:28.000000000 -0600
@@ -62,7 +62,7 @@ static void pci_create_legacy_files(stru
 	}
 }
 
-static void pci_remove_legacy_files(struct pci_bus *b)
+void pci_remove_legacy_files(struct pci_bus *b)
 {
 	class_device_remove_bin_file(&b->class_dev, b->legacy_io);
 	class_device_remove_bin_file(&b->class_dev, b->legacy_mem);
@@ -70,7 +70,7 @@ static void pci_remove_legacy_files(stru
 }
 #else /* !HAVE_PCI_LEGACY */
 static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
-static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
+void pci_remove_legacy_files(struct pci_bus *bus) { return; }
 #endif /* HAVE_PCI_LEGACY */
 
 /*
@@ -86,7 +86,7 @@ static ssize_t pci_bus_show_cpuaffinity(
 		buf[ret++] = '\n';
 	return ret;
 }
-static CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
+CLASS_DEVICE_ATTR(cpuaffinity, S_IRUGO, pci_bus_show_cpuaffinity, NULL);
 
 /*
  * PCI Bus Class
@@ -95,10 +95,6 @@ static void release_pcibus_dev(struct cl
 {
 	struct pci_bus *pci_bus = to_pci_bus(class_dev);
 
-	pci_remove_legacy_files(pci_bus);
-	class_device_remove_file(&pci_bus->class_dev,
-				 &class_device_attr_cpuaffinity);
-	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
 	if (pci_bus->bridge)
 		put_device(pci_bus->bridge);
 	kfree(pci_bus);
diff -puN drivers/pci/remove.c~01_release_pcibus_dev drivers/pci/remove.c
--- 2_6_linus_2/drivers/pci/remove.c~01_release_pcibus_dev	2005-01-13 14:49:21.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/remove.c	2005-01-13 14:49:21.000000000 -0600
@@ -61,15 +61,18 @@ int pci_remove_device_safe(struct pci_de
 }
 EXPORT_SYMBOL(pci_remove_device_safe);
 
-void pci_remove_bus(struct pci_bus *b)
+void pci_remove_bus(struct pci_bus *pci_bus)
 {
-	pci_proc_detach_bus(b);
+	pci_proc_detach_bus(pci_bus);
 
 	spin_lock(&pci_bus_lock);
-	list_del(&b->node);
+	list_del(&pci_bus->node);
 	spin_unlock(&pci_bus_lock);
-
-	class_device_unregister(&b->class_dev);
+	pci_remove_legacy_files(pci_bus);
+	class_device_remove_file(&pci_bus->class_dev,
+		&class_device_attr_cpuaffinity);
+	sysfs_remove_link(&pci_bus->class_dev.kobj, "bridge");
+	class_device_unregister(&pci_bus->class_dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
diff -puN drivers/pci/pci.h~01_release_pcibus_dev drivers/pci/pci.h
--- 2_6_linus_2/drivers/pci/pci.h~01_release_pcibus_dev	2005-01-13 14:49:21.000000000 -0600
+++ 2_6_linus_2-johnrose/drivers/pci/pci.h	2005-01-13 14:50:06.000000000 -0600
@@ -59,12 +59,14 @@ struct pci_visit {
 extern int pci_visit_dev(struct pci_visit *fn,
 			 struct pci_dev_wrapped *wrapped_dev,
 			 struct pci_bus_wrapped *wrapped_parent);
+extern void pci_remove_legacy_files(struct pci_bus *bus);
 
 /* Lock for read/write access to pci device and bus lists */
 extern spinlock_t pci_bus_lock;
 
 extern int pcie_mch_quirk;
 extern struct device_attribute pci_dev_attrs[];
+extern struct class_device_attribute class_device_attr_cpuaffinity;
 
 /**
  * pci_match_one_device - Tell if a PCI device structure has a matching

_

Re: [PATCH] release_pcibus_dev() crash

[John Rose]

> Just to be a brat, I'll point out that I couldn't find a single user of
> CLASS_DEVICE_ATTR that explicitly cleans things up like we're doing here.  That
> would include firmware and net-sysfs stuff.  Maybe enforcing such a policy upon
> device removal would increase participation :)  But okay, here's another try:

Well, okay, I found three drivers that do.  But most don't! :)  </rant> 

John

Re: [PATCH] release_pcibus_dev() crash

[Greg KH]

On Thu, Jan 13, 2005 at 03:17:58PM -0600, John Rose wrote:
> > Closer (you forgot a changelog entry too...)
> 
> Just to be a brat, I'll point out that I couldn't find a single user of
> CLASS_DEVICE_ATTR that explicitly cleans things up like we're doing here.  That
> would include firmware and net-sysfs stuff.  Maybe enforcing such a policy upon
> device removal would increase participation :)  But okay, here's another try:

Yeah, I know everyone doesn't do it, but I'm trying to get them all to,
and when I have a chance to point it out and fix it, I am.  Just like
now, thanks for putting up with me :)

> During the course of a hotplug removal of a PCI bus, release_pcibus_dev()
> attempts to remove attribute files from a kobject directory that no longer
> exists.  This patch moves these calls to pci_remove_bus(), where they can work
> as intended.
> 
> Signed-off-by: John Rose <johnrose@austin.ibm.com>

Looks good, applied, thanks.

greg k-h