[PATCH] PPC64 Fix unbalanced pci_dev_put in EEH code

[PATCH] PPC64 Fix unbalanced pci_dev_put in EEH code

[Paul Mackerras]

The EEH code currently can end up doing an extra pci_dev_put() in the
case where we hot-unplug a card for which we are ignoring EEH errors
(e.g. a graphics card).  This patch fixes that problem by only
maintaining a reference to the PCI device if we have entered any of
its resource addresses into our address -> PCI device cache.  This
patch is based on an earlier patch by Linas Vepstas.

Signed-off-by: Paul Mackerras <paulus@samba.org>

diff -urN linux-2.5/arch/ppc64/kernel/eeh.c g5-ppc64/arch/ppc64/kernel/eeh.c
--- linux-2.5/arch/ppc64/kernel/eeh.c	2004-08-08 12:05:16.000000000 +1000
+++ g5-ppc64/arch/ppc64/kernel/eeh.c	2004-08-17 20:15:20.345809872 +1000
@@ -209,6 +209,7 @@
 {
 	struct device_node *dn;
 	int i;
+	int inserted = 0;
 
 	dn = pci_device_to_OF_node(dev);
 	if (!dn) {
@@ -242,7 +243,12 @@
 		if (start == 0 || ~start == 0 || end == 0 || ~end == 0)
 			 continue;
 		pci_addr_cache_insert(dev, start, end, flags);
+		inserted = 1;
 	}
+
+	/* If there was nothing to add, the cache has no reference... */
+	if (!inserted)
+		pci_dev_put(dev);
 }
 
 /**
@@ -265,6 +271,7 @@
 static inline void __pci_addr_cache_remove_device(struct pci_dev *dev)
 {
 	struct rb_node *n;
+	int removed = 0;
 
 restart:
 	n = rb_first(&pci_io_addr_cache_root.rb_root);
@@ -274,6 +281,7 @@
 
 		if (piar->pcidev == dev) {
 			rb_erase(n, &pci_io_addr_cache_root.rb_root);
+			removed = 1;
 			kfree(piar);
 			goto restart;
 		}
@@ -281,7 +289,8 @@
 	}
 
 	/* The cache no longer holds its reference to this device... */
-	pci_dev_put(dev);
+	if (removed)
+		pci_dev_put(dev);
 }
 
 /**

Re: [PATCH] PPC64 Fix unbalanced pci_dev_put in EEH code

[Linas Vepstas]

Hi Paul,

Hang on there, you didn't just rename the variable, you also missed a
teeny chunk of the original patch.  The corrected path is attached below.
(Well, I rather liked my original var names better, but whatever).

--linas

Signed-off-by: Linas Vepstas <linas@linas.org>

On Wed, Aug 18, 2004 at 01:43:14PM +1000, Paul Mackerras was heard to remark:
> The EEH code currently can end up doing an extra pci_dev_put() in the
> case where we hot-unplug a card for which we are ignoring EEH errors
> (e.g. a graphics card).  This patch fixes that problem by only
> maintaining a reference to the PCI device if we have entered any of
> its resource addresses into our address -> PCI device cache.  This
> patch is based on an earlier patch by Linas Vepstas.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> 


===== arch/ppc64/kernel/eeh.c 1.28 vs edited =====
--- 1.28/arch/ppc64/kernel/eeh.c	Mon Jul 12 18:29:16 2004
+++ edited/arch/ppc64/kernel/eeh.c	Wed Jul 14 15:40:47 2004
@@ -208,6 +208,7 @@
 static void __pci_addr_cache_insert_device(struct pci_dev *dev)
 {
 	struct device_node *dn;
+	int inserted = 0;
 	int i;
 
 	dn = pci_device_to_OF_node(dev);
@@ -226,9 +227,6 @@
 #endif
 		return;
 	}
-
-	/* The cache holds a reference to the device... */
-	pci_dev_get(dev);
 
 	/* Walk resources on this device, poke them into the tree */
 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
@@ -242,6 +242,12 @@
 		if (start == 0 || ~start == 0 || end == 0 || ~end == 0)
 			 continue;
 		pci_addr_cache_insert(dev, start, end, flags);
+		inserted = 1;
+	}
+
+	/* The cache holds a reference to the device... */
+	if (inserted) {
+		pci_dev_get (dev);
 	}
 }
 
@@ -265,6 +270,7 @@
 static inline void __pci_addr_cache_remove_device(struct pci_dev *dev)
 {
 	struct rb_node *n;
+	int removed = 0;
 
 restart:
 	n = rb_first(&pci_io_addr_cache_root.rb_root);
@@ -275,13 +281,16 @@
 		if (piar->pcidev == dev) {
 			rb_erase(n, &pci_io_addr_cache_root.rb_root);
 			kfree(piar);
+			removed = 1;
 			goto restart;
 		}
 		n = rb_next(n);
 	}
 
 	/* The cache no longer holds its reference to this device... */
-	pci_dev_put(dev);
+	if (removed) {
+	   pci_dev_put(dev);
+	}
 }
 
 /**

Re: [PATCH] PPC64 Fix unbalanced pci_dev_put in EEH code

[Paul Mackerras]

Linas Vepstas writes:

> Hang on there, you didn't just rename the variable, you also missed a
> teeny chunk of the original patch.  The corrected path is attached below.
> (Well, I rather liked my original var names better, but whatever).

Look closer, I didn't miss the chunk, I solved the race condition that
you mentioned. :)  I did if (!inserted) pci_dev_put(dev) at the end
rather than if (inserted) pci_dev_get(dev).

Paul.

Re: [PATCH] PPC64 Fix unbalanced pci_dev_put in EEH code

[Linas Vepstas]

On Thu, Aug 19, 2004 at 11:04:14AM +1000, Paul Mackerras was heard to remark:
> Linas Vepstas writes:
> 
> > Hang on there, you didn't just rename the variable, you also missed a
> > teeny chunk of the original patch.  The corrected path is attached below.
> > (Well, I rather liked my original var names better, but whatever).
> 
> Look closer, I didn't miss the chunk, I solved the race condition that
> you mentioned. :)  I did if (!inserted) pci_dev_put(dev) at the end
> rather than if (inserted) pci_dev_get(dev).

OK, I missed the !  (which is why I like !=0 because that avoids
single-char errors from ruinging things.)

That will work.

There's no race, since the for-each-device loop performs a pci_dev_get
which holds on to the device for the duration of the loop interior.
So doing a dev_get at any time inside the loop is sufficient; no
ordering is required.

(One of my original errors was assuming that I had to do a dev_put at the
bottom of the loop to counteract the for-each-device's get; but, no, 
this is not needed, the for-each-device loop performs a dev-put at 
the end of the loop, just as it does a dev_get for the next iteration.)

--linas