[PATCH 7/15] EDAC: i82875p cleanup

[PATCH 7/15] EDAC: i82875p cleanup

[Dave Peterson]

- Fix i82875p_probe1() so it calls pci_get_device() instead of
  pci_find_device().
- Fix i82875p_probe1() so it cleans up properly on failure.
- Fix i82875p_init() so it cleans up properly on failure.

Signed-Off-By: David S. Peterson <dsp@llnl.gov> <dave_peterson@pobox.com>
---

Index: linux-2.6.16-rc5-edac/drivers/edac/i82875p_edac.c
===================================================================
--- linux-2.6.16-rc5-edac.orig/drivers/edac/i82875p_edac.c	2006-02-27 16:58:41.000000000 -0800
+++ linux-2.6.16-rc5-edac/drivers/edac/i82875p_edac.c	2006-02-27 17:04:31.000000000 -0800
@@ -289,7 +289,7 @@ static int i82875p_probe1(struct pci_dev
 
 	debugf0("%s()\n", __func__);
 
-	ovrfl_pdev = pci_find_device(PCI_VEND_DEV(INTEL, 82875_6), NULL);
+	ovrfl_pdev = pci_get_device(PCI_VEND_DEV(INTEL, 82875_6), NULL);
 
 	if (!ovrfl_pdev) {
 		/*
@@ -302,26 +302,26 @@ static int i82875p_probe1(struct pci_dev
 		ovrfl_pdev =
 		    pci_scan_single_device(pdev->bus, PCI_DEVFN(6, 0));
 		if (!ovrfl_pdev)
-			goto fail;
+			return -ENODEV;
 	}
 #ifdef CONFIG_PROC_FS
 	if (!ovrfl_pdev->procent && pci_proc_attach_device(ovrfl_pdev)) {
 		i82875p_printk(KERN_ERR,
 			       "%s(): Failed to attach overflow device\n",
 			       __func__);
-		goto fail;
+		return -ENODEV;
 	}
 #endif				/* CONFIG_PROC_FS */
 	if (pci_enable_device(ovrfl_pdev)) {
 		i82875p_printk(KERN_ERR,
 			       "%s(): Failed to enable overflow device\n",
 			       __func__);
-		goto fail;
+		return -ENODEV;
 	}
 
 	if (pci_request_regions(ovrfl_pdev, pci_name(ovrfl_pdev))) {
 #ifdef CORRECT_BIOS
-		goto fail;
+		goto fail0;
 #endif
 	}
 	/* cache is irrelevant for PCI bus reads/writes */
@@ -331,7 +331,7 @@ static int i82875p_probe1(struct pci_dev
 	if (!ovrfl_window) {
 		i82875p_printk(KERN_ERR, "%s(): Failed to ioremap bar6\n",
 			       __func__);
-		goto fail;
+		goto fail1;
 	}
 
 	/* need to find out the number of channels */
@@ -345,7 +345,7 @@ static int i82875p_probe1(struct pci_dev
 
 	if (!mci) {
 		rc = -ENOMEM;
-		goto fail;
+		goto fail2;
 	}
 
 	debugf3("%s(): init mci\n", __func__);
@@ -402,25 +402,26 @@ static int i82875p_probe1(struct pci_dev
 
 	if (edac_mc_add_mc(mci)) {
 		debugf3("%s(): failed edac_mc_add_mc()\n", __func__);
-		goto fail;
+		goto fail3;
 	}
 
 	/* get this far and it's successful */
 	debugf3("%s(): success\n", __func__);
 	return 0;
 
-      fail:
-	if (mci)
-		edac_mc_free(mci);
-
-	if (ovrfl_window)
-		iounmap(ovrfl_window);
-
-	if (ovrfl_pdev) {
-		pci_release_regions(ovrfl_pdev);
-		pci_disable_device(ovrfl_pdev);
-	}
+fail3:
+	edac_mc_free(mci);
+
+fail2:
+	iounmap(ovrfl_window);
 
+fail1:
+	pci_release_regions(ovrfl_pdev);
+
+#ifdef CORRECT_BIOS
+fail0:
+#endif
+	pci_disable_device(ovrfl_pdev);
 	/* NOTE: the ovrfl proc entry and pci_dev are intentionally left */
 	return rc;
 }
@@ -497,24 +498,33 @@ static int __init i82875p_init(void)
 	debugf3("%s()\n", __func__);
 	pci_rc = pci_register_driver(&i82875p_driver);
 	if (pci_rc < 0)
-		return pci_rc;
+		goto fail0;
 	if (mci_pdev == NULL) {
-		i82875p_registered = 0;
 		mci_pdev =
 		    pci_get_device(PCI_VENDOR_ID_INTEL,
 				   PCI_DEVICE_ID_INTEL_82875_0, NULL);
 		if (!mci_pdev) {
 			debugf0("875p pci_get_device fail\n");
-			return -ENODEV;
+			pci_rc = -ENODEV;
+			goto fail1;
 		}
 		pci_rc = i82875p_init_one(mci_pdev, i82875p_pci_tbl);
 		if (pci_rc < 0) {
 			debugf0("875p init fail\n");
-			pci_dev_put(mci_pdev);
-			return -ENODEV;
+			pci_rc = -ENODEV;
+			goto fail1;
 		}
 	}
 	return 0;
+
+fail1:
+	pci_unregister_driver(&i82875p_driver);
+
+fail0:
+	if (mci_pdev != NULL)
+		pci_dev_put(mci_pdev);
+
+	return pci_rc;
 }
 
 

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Andrew Morton]

Dave Peterson <dsp@llnl.gov> wrote:
>
>  +#ifdef CORRECT_BIOS
>  +fail0:
>  +#endif

What is CORRECT_BIOS?  Is the fact that it's never defined some sort of
commentary?  ;)

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Dave Peterson]

On Thursday 02 March 2006 18:30, Andrew Morton wrote:
> Dave Peterson <dsp@llnl.gov> wrote:
> >  +#ifdef CORRECT_BIOS
> >  +fail0:
> >  +#endif
>
> What is CORRECT_BIOS?  Is the fact that it's never defined some sort of
> commentary?  ;)

I'm not sure about this.  I'm cc'ing Thayne Harbaugh and Wang Zhenyu since
their names are in the credits for the i82875p module.  Maybe they can
provide some info.

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Thayne Harbaugh]

On Fri, 2006-03-03 at 10:47 -0800, Dave Peterson wrote:
> On Thursday 02 March 2006 18:30, Andrew Morton wrote:
> > Dave Peterson <dsp@llnl.gov> wrote:
> > >  +#ifdef CORRECT_BIOS
> > >  +fail0:
> > >  +#endif
> >
> > What is CORRECT_BIOS?  Is the fact that it's never defined some sort of
> > commentary?  ;)
> 
> I'm not sure about this.  I'm cc'ing Thayne Harbaugh and Wang Zhenyu since
> their names are in the credits for the i82875p module.  Maybe they can
> provide some info.

This is something that is my style - I don't care for "#if 0" or "#if
1".  I usually drop a "#undef COMMENT_TAG" someplace with a "/* ... */"
comment next to it so that it's not some unknown tag.

I haven't looked through the code yet so I can't remember if it's
something I left and if it is, what it does.

I just looked, and I don't recognize it - "cvs annotate" lists it ass
being last touched by dsp_llnl ;^).


-- 
A silly version!
so I fix the stripping beat
with bitter sleep snot.

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Henrique de Moraes Holschuh]

On Fri, 03 Mar 2006, Thayne Harbaugh wrote:
> On Fri, 2006-03-03 at 10:47 -0800, Dave Peterson wrote:
> > On Thursday 02 March 2006 18:30, Andrew Morton wrote:
> > > Dave Peterson <dsp@llnl.gov> wrote:
> > > >  +#ifdef CORRECT_BIOS
> > > >  +fail0:
> > > >  +#endif
> > >
> > > What is CORRECT_BIOS?  Is the fact that it's never defined some sort of
> > > commentary?  ;)
> > 
> > I'm not sure about this.  I'm cc'ing Thayne Harbaugh and Wang Zhenyu since
> > their names are in the credits for the i82875p module.  Maybe they can
> > provide some info.
[cut]
> 
> I haven't looked through the code yet so I can't remember if it's
> something I left and if it is, what it does.
> 
> I just looked, and I don't recognize it - "cvs annotate" lists it ass
> being last touched by dsp_llnl ;^).

Maybe it is a left-over of that bogus warning about the BIOS reserving the
region used by the hidden pci device (device 0:0:06.0)?  Consensus was that
if the BIOS hides the device, it is supposed to reserve that area since it
is indeed in use by the hidden, but still active, 82875P configuration-space
overflow PCI device, so no warnings were required.

The current code still spills useless warnings, but that's because it tries
to reserve the pci resources and the pci code itself outputs warnings.
Maybe it should query if that resource is already reserved, and not try to
reserve it in that case (to avoid the warning).

Also, last time I checked (latest bluesmoke code, didn't try EDAC yet), the
code unhides the PCI 0:0:06.0 device so as to do the required EDAC setup and
pooling, but does not do whatever is needed to add that device to the
regular pci device list used by lspci.

Here's the device I am talking about (lspci -H1 finds it):
0000:00:06.0 System peripheral: Intel Corporation 82875P/E7210 Processor to
I/O Memory Interface (rev 02)
        Flags: fast devsel
	Memory at fecf0000 (32-bit, non-prefetchable)

Just plain lspci (even as root) won't list it.  If EDAC/bluesmoke is not
loaded, the device is kept hidden and not even lspci -H1 can find it.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Wang Zhenyu]

On 2006.03.04 02:47:01 +0000, Dave Peterson wrote:
> 
>    On Thursday 02 March 2006 18:30, Andrew Morton wrote:
>    > Dave Peterson <dsp@llnl.gov> wrote:
>    > >  +#ifdef CORRECT_BIOS
>    > >  +fail0:
>    > >  +#endif
>    >
>    > What is CORRECT_BIOS?  Is the fact that it's never defined some sort of
>    > commentary?  ;)
>    I'm not sure about this.  I'm cc'ing Thayne Harbaugh and Wang Zhenyu since
>    their names are in the credits for the i82875p module.  Maybe they can
>    provide some info.

You can take CORRECT_BIOS as "strict-pci-resource-reserve" for overflow device
in 82875p, some bad BIOS does make it reserved, which cause pci_request_region()
failed.  Actually we never defined it. 

zhen

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Henrique de Moraes Holschuh]

On Tue, 07 Mar 2006, Wang Zhenyu wrote:
> On 2006.03.04 02:47:01 +0000, Dave Peterson wrote:
> >    On Thursday 02 March 2006 18:30, Andrew Morton wrote:
> >    > Dave Peterson <dsp@llnl.gov> wrote:
> >    > >  +#ifdef CORRECT_BIOS
> >    > >  +fail0:
> >    > >  +#endif
> >    >
> >    > What is CORRECT_BIOS?  Is the fact that it's never defined some sort of
> >    > commentary?  ;)
> >    I'm not sure about this.  I'm cc'ing Thayne Harbaugh and Wang Zhenyu since
> >    their names are in the credits for the i82875p module.  Maybe they can
> >    provide some info.
> 
> You can take CORRECT_BIOS as "strict-pci-resource-reserve" for overflow device
> in 82875p, some bad BIOS does make it reserved, which cause pci_request_region()
> failed.  Actually we never defined it. 

Bad? :-)  It would be bad only if they didn't *hide* the overflow device.
Regardless of the overflow device being hidden or not, that area is really
in use, and should be known to be in use.  How can you know it is in use if
the device is hidden, unless the BIOS reserves it?

Let's call that "inconvenient" BIOSes instead...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 7/15] EDAC: i82875p cleanup

[Andrew Morton]

Dave Peterson <dsp@llnl.gov> wrote:
>
> +	if (mci_pdev != NULL)
>  +		pci_dev_put(mci_pdev);

pci_dev_put(NULL) is legal.