diff mbox

3x59x-fix-pci-resource-management.patch

Message ID 20090806154223.b9b9759e.akpm@linux-foundation.org
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Andrew Morton Aug. 6, 2009, 10:42 p.m. UTC 
Guys, I seem to have been sitting on the below patch for, err, three years.

I generally do this because there's some issue which needs to be
addressed, but the patch didn't address it right.

Can we please have a revisit and work out what we should do with this?

Thanks.


From: Sergei Shtylyov <sshtylyov@ru.mvista.com>

The driver wrongly claimed I/O ports at an address returned by pci_iomap()
-- even if it was passed an MMIO address.  Fix this by claiming/releasing
all PCI resources in the PCI driver probe/remove handlers instead and get
rid of the must_free_region flag weirdness (why would Cardbus claim
anything for us?).

Also, the remove handler was trying to talk to the chip after having
disabled its address decoders (at least on x86) -- fix this and get rid of
useless VORTEX_PCI() calls.

While at it, fix some cases of the overly indented code...

[akpm@linux-foundation.org: coding-style fixes]
Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Steffen Klassert <klassert@mathematik.tu-chemnitz.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/3c59x.c |   66 ++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

Comments

Steffen Klassert Aug. 7, 2009, 8:19 a.m. UTC | #1 
On Thu, Aug 06, 2009 at 03:42:23PM -0700, Andrew Morton wrote:
> 
> Guys, I seem to have been sitting on the below patch for, err, three years.
> 
> I generally do this because there's some issue which needs to be
> addressed, but the patch didn't address it right.
> 
> Can we please have a revisit and work out what we should do with this?
> 

I don't know the story from the beginning. Are there any issues with
this patch aside the compile error that was caused if CONFIG_PCI is not
enabled? The compile error should be fixed and the rest looks ok on the
first view.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton Aug. 7, 2009, 8:33 a.m. UTC | #2 
On Fri, 7 Aug 2009 10:19:28 +0200 Steffen Klassert <klassert@mathematik.tu-chemnitz.de> wrote:

> On Thu, Aug 06, 2009 at 03:42:23PM -0700, Andrew Morton wrote:
> > 
> > Guys, I seem to have been sitting on the below patch for, err, three years.
> > 
> > I generally do this because there's some issue which needs to be
> > addressed, but the patch didn't address it right.
> > 
> > Can we please have a revisit and work out what we should do with this?
> > 
> 
> I don't know the story from the beginning.

I've forgotten.  I don't recall seeing any bug reports which this patch
might fix.

> Are there any issues with
> this patch aside the compile error that was caused if CONFIG_PCI is not
> enabled? The compile error should be fixed and the rest looks ok on the
> first view.

I just did a PCI=n, EISA=y build and it compiled OK.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steffen Klassert Aug. 7, 2009, 10:01 a.m. UTC | #3 
On Thu, Aug 06, 2009 at 03:42:23PM -0700, Andrew Morton wrote:
>  	/* PCI-only startup logic */
>  	if (pdev) {
> -		/* EISA resources already marked, so only PCI needs to do this here */
> -		/* Ignore return value, because Cardbus drivers already allocate for us */
> -		if (request_region(dev->base_addr, vci->io_size, print_name) != NULL)
> -			vp->must_free_region = 1;
> -

I don't know what cardbus does, but __request_region() returns NULL on
memory allocation failure for struct resource too. So this did not work
in all cases anyway. If cardbus allocates for us, we should handle it in
some way, if not I'm fine with the patch as it is.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Aug. 7, 2009, 10:53 a.m. UTC | #4 
Hello.

Andrew Morton wrote:

> Guys, I seem to have been sitting on the below patch for, err, three years.

> I generally do this because there's some issue which needs to be
> addressed, but the patch didn't address it right.

    In this case it was different: you just wanted the patch to be verifed 
on an EISA card...

On Mon, 14 Aug 2006 14:31:46 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

 >> ACK, shall I apply this to netdev#upstream ?

Spose so, but let's not go mainline until I've had a chance to test an
EISA card and a cardbus card.

    There was several more comments from you but they all got resolved 
between you and Jeff at the time...

> Can we please have a revisit and work out what we should do with this?

> Thanks.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton Aug. 7, 2009, 4:14 p.m. UTC | #5 
On Fri, 07 Aug 2009 14:53:17 +0400 Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Hello.
> 
> Andrew Morton wrote:
> 
> > Guys, I seem to have been sitting on the below patch for, err, three years.
> 
> > I generally do this because there's some issue which needs to be
> > addressed, but the patch didn't address it right.
> 
>     In this case it was different: you just wanted the patch to be verifed 
> on an EISA card...
> 
> On Mon, 14 Aug 2006 14:31:46 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>  >> ACK, shall I apply this to netdev#upstream ?
> 
> Spose so, but let's not go mainline until I've had a chance to test an
> EISA card and a cardbus card.
> 
>     There was several more comments from you but they all got resolved 
> between you and Jeff at the time...
> 

Doh, OK, thanks.  My fault.

I have a cardbus card somewhere..
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 13, 2009, 12:26 a.m. UTC | #6 
From: Andrew Morton <akpm@linux-foundation.org>
Date: Fri, 7 Aug 2009 09:14:15 -0700

> On Fri, 07 Aug 2009 14:53:17 +0400 Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> 
>>     There was several more comments from you but they all got resolved 
>> between you and Jeff at the time...
>> 
> 
> Doh, OK, thanks.  My fault.
> 
> I have a cardbus card somewhere..

Please repost this when we have at least some cardbus
testing of some sort, because if we can get rid of all
of that resource special casing that would be nice.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff -puN drivers/mtd/devices/Kconfig~3x59x-fix-pci-resource-management drivers/mtd/devices/Kconfig
diff -puN drivers/mtd/devices/Makefile~3x59x-fix-pci-resource-management drivers/mtd/devices/Makefile
diff -puN drivers/mtd/devices/sst25l.c~3x59x-fix-pci-resource-management drivers/mtd/devices/sst25l.c
diff -puN drivers/net/3c59x.c~3x59x-fix-pci-resource-management drivers/net/3c59x.c
--- a/drivers/net/3c59x.c~3x59x-fix-pci-resource-management
+++ a/drivers/net/3c59x.c
@@ -629,7 +629,6 @@  struct vortex_private {
 		pm_state_valid:1,				/* pci_dev->saved_config_space has sane contents */
 		open:1,
 		medialock:1,
-		must_free_region:1,				/* Flag: if zero, Cardbus owns the I/O region */
 		large_frames:1;			/* accept large frames */
 	int drv_flags;
 	u16 status_enable;
@@ -962,7 +961,13 @@  static int __devinit vortex_init_one(str
 	/* wake up and enable device */
 	rc = pci_enable_device(pdev);
 	if (rc < 0)
-		goto out;
+		return rc;
+
+	rc = pci_request_regions(pdev, "3c59x");
+	if (rc < 0) {
+		pci_disable_device(pdev);
+		return rc;
+	}
 
 	unit = vortex_cards_found;
 
@@ -982,13 +987,13 @@  static int __devinit vortex_init_one(str
 	rc = vortex_probe1(&pdev->dev, ioaddr, pdev->irq,
 			   ent->driver_data, unit);
 	if (rc < 0) {
+		pci_release_regions(pdev);
 		pci_disable_device(pdev);
-		goto out;
+		return rc;
 	}
 
 	vortex_cards_found++;
 
-out:
 	return rc;
 }
 
@@ -1125,11 +1130,6 @@  static int __devinit vortex_probe1(struc
 
 	/* PCI-only startup logic */
 	if (pdev) {
-		/* EISA resources already marked, so only PCI needs to do this here */
-		/* Ignore return value, because Cardbus drivers already allocate for us */
-		if (request_region(dev->base_addr, vci->io_size, print_name) != NULL)
-			vp->must_free_region = 1;
-
 		/* enable bus-mastering if necessary */
 		if (vci->flags & PCI_USES_MASTER)
 			pci_set_master(pdev);
@@ -1160,12 +1160,13 @@  static int __devinit vortex_probe1(struc
 	vp->mii.reg_num_mask = 0x1f;
 
 	/* Makes sure rings are at least 16 byte aligned. */
-	vp->rx_ring = pci_alloc_consistent(pdev, sizeof(struct boom_rx_desc) * RX_RING_SIZE
-					   + sizeof(struct boom_tx_desc) * TX_RING_SIZE,
-					   &vp->rx_ring_dma);
+	vp->rx_ring = pci_alloc_consistent(pdev,
+				sizeof(struct boom_rx_desc) * RX_RING_SIZE +
+				sizeof(struct boom_tx_desc) * TX_RING_SIZE,
+				&vp->rx_ring_dma);
 	retval = -ENOMEM;
 	if (!vp->rx_ring)
-		goto free_region;
+		goto free_device;
 
 	vp->tx_ring = (struct boom_tx_desc *)(vp->rx_ring + RX_RING_SIZE);
 	vp->tx_ring_dma = vp->rx_ring_dma + sizeof(struct boom_rx_desc) * RX_RING_SIZE;
@@ -1432,13 +1433,11 @@  static int __devinit vortex_probe1(struc
 
 free_ring:
 	pci_free_consistent(pdev,
-						sizeof(struct boom_rx_desc) * RX_RING_SIZE
-							+ sizeof(struct boom_tx_desc) * TX_RING_SIZE,
-						vp->rx_ring,
-						vp->rx_ring_dma);
-free_region:
-	if (vp->must_free_region)
-		release_region(dev->base_addr, vci->io_size);
+			    sizeof(struct boom_rx_desc) * RX_RING_SIZE +
+			    sizeof(struct boom_tx_desc) * TX_RING_SIZE,
+			    vp->rx_ring,
+			    vp->rx_ring_dma);
+free_device:
 	free_netdev(dev);
 	pr_err(PFX "vortex_probe1 fails.  Returns %d\n", retval);
 out:
@@ -3167,29 +3166,28 @@  static void __devexit vortex_remove_one(
 	vp = netdev_priv(dev);
 
 	if (vp->cb_fn_base)
-		pci_iounmap(VORTEX_PCI(vp), vp->cb_fn_base);
+		pci_iounmap(pdev, vp->cb_fn_base);
 
 	unregister_netdev(dev);
 
-	if (VORTEX_PCI(vp)) {
-		pci_set_power_state(VORTEX_PCI(vp), PCI_D0);	/* Go active */
-		if (vp->pm_state_valid)
-			pci_restore_state(VORTEX_PCI(vp));
-		pci_disable_device(VORTEX_PCI(vp));
-	}
+	pci_set_power_state(pdev, PCI_D0);	/* Go active */
+	if (vp->pm_state_valid)
+		pci_restore_state(pdev);
+
 	/* Should really use issue_and_wait() here */
 	iowrite16(TotalReset | ((vp->drv_flags & EEPROM_RESET) ? 0x04 : 0x14),
 	     vp->ioaddr + EL3_CMD);
 
-	pci_iounmap(VORTEX_PCI(vp), vp->ioaddr);
+	pci_iounmap(pdev, vp->ioaddr);
 
 	pci_free_consistent(pdev,
-						sizeof(struct boom_rx_desc) * RX_RING_SIZE
-							+ sizeof(struct boom_tx_desc) * TX_RING_SIZE,
-						vp->rx_ring,
-						vp->rx_ring_dma);
-	if (vp->must_free_region)
-		release_region(dev->base_addr, vp->io_size);
+			    sizeof(struct boom_rx_desc) * RX_RING_SIZE +
+			    sizeof(struct boom_tx_desc) * TX_RING_SIZE,
+			    vp->rx_ring,
+			    vp->rx_ring_dma);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
 	free_netdev(dev);
 }