diff mbox

[v2] 3c59x: fix PCI resource management

Message ID 201305020240.34487.sergei.shtylyov@cogentembedded.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Sergei Shtylyov May 1, 2013, 10:40 p.m. UTC 
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() methods instead and get rid of the
'must_free_region' flag weirdness (why would Cardbus claim anything for us?).

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

While at it, fix some cases of the overly indented code and the code crossing
a 80-column boundary...

[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>

---
The patch is against David Miller's 'net-next.git' repo.
Matthew, please test it on one of your 3c59x EISA boards.
Steffen, here's my second try to get this patch past you, the last one was back
in 2009...

Changes since 2009:
- don't change *goto* to *return* in vortex_init_one();
- added a new pci_release_regions() call in pci_iomap() error cleanup code;
- used DRV_NAME in pci_request_regions() call;
- somewhat rephrased the changelog.

 drivers/net/ethernet/3com/3c59x.c |   62 +++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

--
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

Comments

Sergei Shtylyov May 1, 2013, 10:49 p.m. UTC | #1 
Hello.

On 05/02/2013 02:40 AM, Sergei Shtylyov wrote:

> 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() methods instead and get rid of the
> 'must_free_region' flag weirdness (why would Cardbus claim anything for us?).
>
> Also, the remove() method was trying to talk to the chip after having disabled
> its address decoders (at least on x86) -- fix this and while doing it get rid
> of the useless VORTEX_PCI() invocations.
>
> While at it, fix some cases of the overly indented code and the code crossing
> a 80-column boundary...
>
> [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>
>
> ---
> The patch is against David Miller's 'net-next.git' repo.
> Matthew, please test it on one of your 3c59x EISA boards.
> Steffen, here's my second try to get this patch past you, the last one was back
> in 2009...

     This is mostly a request for testing at this point, I forgot to add 
[RFT]
to the subject...

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
Sergei Shtylyov May 2, 2013, 5:40 p.m. UTC | #2 
Hello.

On 02-05-2013 2:49, Sergei Shtylyov wrote:

>> 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() methods instead and get rid of the
>> 'must_free_region' flag weirdness (why would Cardbus claim anything for us?).

>> Also, the remove() method was trying to talk to the chip after having disabled
>> its address decoders (at least on x86) -- fix this and while doing it get rid
>> of the useless VORTEX_PCI() invocations.

>> While at it, fix some cases of the overly indented code and the code crossing
>> a 80-column boundary...

>> [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>

>> ---
>> The patch is against David Miller's 'net-next.git' repo.
>> Matthew, please test it on one of your 3c59x EISA boards.
>> Steffen, here's my second try to get this patch past you, the last one was back
>> in 2009...

>      This is mostly a request for testing at this point, I forgot to add [RFT]
> to the subject...

    David and others, your comments are welcome however. The patch probably 
needs some more trimming before it can be considered a pure fix. It has been 
somewhat tested now on EISA (and once more on PCI) board, and as a result of 
testing another one, EISA specific error was reported, fix for which I'm going 
to publish for testing RSN.

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
David Miller May 6, 2013, 4:20 p.m. UTC | #3 
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Thu, 2 May 2013 02:40:34 +0400

> @@ -1215,12 +1217,13 @@ static int vortex_probe1(struct device *
>  	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);

This code was properly indented before your changes, but it isn't afterwards.

Function calls spanning multiple lines must align the arguments on the
second and subsequent lines at the first column after the openning
parenthesis of the function call, using the appropriate number of
TAB and space characters.

So:

		func(a, b, c,
		     d, e, f);

not:

		func(a, b, c,
			d, e, f);

The objective is to align at the correct column, rather than exclusively
using TAB characters.
--
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 May 6, 2013, 5:29 p.m. UTC | #4 
Hello.

On 06-05-2013 20:20, David Miller wrote:

>> @@ -1215,12 +1217,13 @@ static int vortex_probe1(struct device *
>>   	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);

> This code was properly indented before your changes, but it isn't afterwards.

    David, that's not my change, it's was Andrew Morton who changed that 
(in order not to cross 80 columns).

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
David Miller May 6, 2013, 5:53 p.m. UTC | #5 
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 06 May 2013 21:29:40 +0400

> Hello.
> 
> On 06-05-2013 20:20, David Miller wrote:
> 
>>> @@ -1215,12 +1217,13 @@ static int vortex_probe1(struct device *
>>>   	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);
> 
>> This code was properly indented before your changes, but it isn't
>> afterwards.
> 
>    David, that's not my change, it's was Andrew Morton who changed that
>    (in order not to cross 80 columns).

Ok, whoever submits this next needs to undo that.
--
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 May 6, 2013, 6:23 p.m. UTC | #6 
Hello.

On 06-05-2013 21:53, David Miller wrote:

>>>> @@ -1215,12 +1217,13 @@ static int vortex_probe1(struct device *
>>>>    	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);

>>> This code was properly indented before your changes, but it isn't
>>> afterwards.

>>     David, that's not my change, it's was Andrew Morton who changed that
>>     (in order not to cross 80 columns).

> Ok, whoever submits this next needs to undo that.

     How about I completely drop this change (and maybe deal with it in 
a subsequent cleanup patch)?

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
David Miller May 6, 2013, 6:59 p.m. UTC | #7 
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 06 May 2013 22:23:14 +0400

>     How about I completely drop this change (and maybe deal with it in a
>     subsequent cleanup patch)?

Sure, that cleanup would have to wait until after the merge window is
closed and I reopen net-next.

Thanks.
--
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 May 6, 2013, 7:33 p.m. UTC | #8 
On 05/06/2013 10:59 PM, David Miller wrote:

>
>>      How about I completely drop this change (and maybe deal with it in a
>>      subsequent cleanup patch)?
> Sure, that cleanup would have to wait until after the merge window is
> closed and I reopen net-next.

    Yes, I know.
    What's your opinion on getting rid of VORTEX_PCI() invocations in 
the PCI
remove() method done in this same patch -- are they worth splitting to a
cleanup patch too?..

> 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
David Miller May 6, 2013, 7:57 p.m. UTC | #9 
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 06 May 2013 23:33:52 +0400

> On 05/06/2013 10:59 PM, David Miller wrote:
> 
>>
>>>      How about I completely drop this change (and maybe deal with it in a
>>>      subsequent cleanup patch)?
>> Sure, that cleanup would have to wait until after the merge window is
>> closed and I reopen net-next.
> 
>    Yes, I know.
>    What's your opinion on getting rid of VORTEX_PCI() invocations in the
>    PCI
> remove() method done in this same patch -- are they worth splitting to
> a
> cleanup patch too?..

Defer to the cleanup patch.
--
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 May 19, 2013, 7:36 p.m. UTC | #10 
Hello.

On 05/06/2013 10:23 PM, Sergei Shtylyov wrote:

>
>>>>> @@ -1215,12 +1217,13 @@ static int vortex_probe1(struct device *
>>>>>        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);
>
>>>> This code was properly indented before your changes, but it isn't
>>>> afterwards.
>
>>>     David, that's not my change, it's was Andrew Morton who changed 
>>> that
>>>     (in order not to cross 80 columns).
>
>> Ok, whoever submits this next needs to undo that.
>
>     How about I completely drop this change (and maybe deal with it in 
> a subsequent cleanup patch)?

    It appears I had fixes for only a small part of 
scripts/checkpatch.pl complaints
in this driver:

total: 114 errors, 428 warnings, 3327 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
       scripts/cleanfile

    I guess you don't need me to clean up the coding style in such an 
old driver,
and I don't want to spend time on it either. So I'm going to let the lot 
of overly
long lines in the driver live forever. :-)

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
diff mbox

Patch

Index: net-next/drivers/net/ethernet/3com/3c59x.c
===================================================================
--- net-next.orig/drivers/net/ethernet/3com/3c59x.c
+++ net-next/drivers/net/ethernet/3com/3c59x.c
@@ -632,7 +632,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 */
 		handling_irq:1;			/* private in_irq indicator */
 	/* {get|set}_wol operations are already serialized by rtnl.
@@ -1012,6 +1011,12 @@  static int vortex_init_one(struct pci_de
 	if (rc < 0)
 		goto out;
 
+	rc = pci_request_regions(pdev, DRV_NAME);
+	if (rc < 0) {
+		pci_disable_device(pdev);
+		goto out;
+	}
+
 	unit = vortex_cards_found;
 
 	if (global_use_mmio < 0 && (unit >= MAX_UNITS || use_mmio[unit] < 0)) {
@@ -1027,6 +1032,7 @@  static int vortex_init_one(struct pci_de
 	if (!ioaddr) /* If mapping fails, fall-back to BAR 0... */
 		ioaddr = pci_iomap(pdev, 0, 0);
 	if (!ioaddr) {
+		pci_release_regions(pdev);
 		pci_disable_device(pdev);
 		rc = -ENOMEM;
 		goto out;
@@ -1036,6 +1042,7 @@  static int vortex_init_one(struct pci_de
 			   ent->driver_data, unit);
 	if (rc < 0) {
 		pci_iounmap(pdev, ioaddr);
+		pci_release_regions(pdev);
 		pci_disable_device(pdev);
 		goto out;
 	}
@@ -1178,11 +1185,6 @@  static int vortex_probe1(struct device *
 
 	/* 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);
@@ -1215,12 +1217,13 @@  static int vortex_probe1(struct device *
 	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;
@@ -1480,13 +1483,11 @@  static int vortex_probe1(struct device *
 
 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:
@@ -3233,29 +3234,28 @@  static void vortex_remove_one(struct pci
 	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);
 }