[PATCH] Don't call request_region() for 3C90x chips

[PATCH] Don't call request_region() for 3C90x chips

[Sergei Shtylylov]

It's generally not a good idea to call request_region() on an address returned 
by pci_iomap(), even less so on a MMIO address. And there was absolutely no 
point in claiming the region already claimed by the PCI core, especially with 
the same PCI generic owner's name. As this is the only case of the 
must_free_region flag being set, this flag may go away as well...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

Index: linux-2.6/drivers/net/3c59x.c
===================================================================
--- linux-2.6.orig/drivers/net/3c59x.c
+++ linux-2.6/drivers/net/3c59x.c
@@ -630,7 +630,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;
@@ -1084,11 +1083,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);
@@ -1125,7 +1119,7 @@ static int __devinit vortex_probe1(struc
 					   &vp->rx_ring_dma);
 	retval = -ENOMEM;
 	if (vp->rx_ring == 0)
-		goto free_region;
+		goto free_netdev;
 
 	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;
@@ -1410,9 +1404,7 @@ free_ring:
 							+ 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);
+free_netdev:
 	free_netdev(dev);
 	printk(KERN_ERR PFX "vortex_probe1 fails.  Returns %d\n", retval);
 out:
@@ -3143,8 +3135,6 @@ static void __devexit vortex_remove_one(
 							+ 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);
 	free_netdev(dev);
 }
 

Re: [PATCH] Don't call request_region() for 3C90x chips

[Jeff Garzik]

Sergei Shtylylov wrote:
> It's generally not a good idea to call request_region() on an address returned 
> by pci_iomap(), even less so on a MMIO address. And there was absolutely no 
> point in claiming the region already claimed by the PCI core, especially with 
> the same PCI generic owner's name. As this is the only case of the 
> must_free_region flag being set, this flag may go away as well...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

I agree you have identified a bug, but this is not a solution.

The current driver bug is that it calls request_region() potentially on 
an MMIO address, but the solution is _not_ to completely avoid reserving 
the resource.

The region registered with the PCI core, but _not_ claimed by anyone. 
Someone still needs to either call pci_{request,release}_regions() or 
request_[mem_]region() to indicate that the resource is reserved.

This bug you have found was probably a missed detail during the 
conversion to the iomap API.

	Jeff

Re: [PATCH] Don't call request_region() for 3C90x chips

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>> It's generally not a good idea to call request_region() on an address 
>> returned by pci_iomap(), even less so on a MMIO address. And there was 
>> absolutely no point in claiming the region already claimed by the PCI 
>> core, especially with the same PCI generic owner's name. As this is 
>> the only case of the must_free_region flag being set, this flag may go 
>> away as well...

>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> I agree you have identified a bug, but this is not a solution.

> The current driver bug is that it calls request_region() potentially on 
> an MMIO address, but the solution is _not_ to completely avoid reserving 
> the resource.

    It's not even a MMIO/PIO address anymore after pci_iomap() -- it either 
went thru ioremap() or ioport_map() which both change the mapping from the 
physical to the virtual address (or some equivalent of it for I/O ports).

> The region registered with the PCI core, but _not_ claimed by anyone. 
> Someone still needs to either call pci_{request,release}_regions() or 
> request_[mem_]region() to indicate that the resource is reserved.

    Sigh, it seems I've missed that difference. So, I'll recast...

> This bug you have found was probably a missed detail during the 
> conversion to the iomap API.

    Well, not only that: it was wrong once the driver started using MMIO 
(which is of course a preference).

>     Jeff

WBR, Sergei

Re: [PATCH] Don't call request_region() for 3C90x chips

[Jeff Garzik]

Sergei Shtylyov wrote:
> Hello.
> 
> Jeff Garzik wrote:
> 
>>> It's generally not a good idea to call request_region() on an address 
>>> returned by pci_iomap(), even less so on a MMIO address. And there 
>>> was absolutely no point in claiming the region already claimed by the 
>>> PCI core, especially with the same PCI generic owner's name. As this 
>>> is the only case of the must_free_region flag being set, this flag 
>>> may go away as well...
> 
>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>> I agree you have identified a bug, but this is not a solution.
> 
>> The current driver bug is that it calls request_region() potentially 
>> on an MMIO address, but the solution is _not_ to completely avoid 
>> reserving the resource.
> 
>    It's not even a MMIO/PIO address anymore after pci_iomap() -- it 
> either went thru ioremap() or ioport_map() which both change the mapping 
> from the physical to the virtual address (or some equivalent of it for 
> I/O ports).

Yes.  _Obviously_ you must reserve the resource passed to 
pci_iomap/ioremap, not the cookie returned by such.


>> The region registered with the PCI core, but _not_ claimed by anyone. 
>> Someone still needs to either call pci_{request,release}_regions() or 
>> request_[mem_]region() to indicate that the resource is reserved.
> 
>    Sigh, it seems I've missed that difference. So, I'll recast...

IMO it would be easiest to do pci_{request,release}_regions() in the 
PCI-only code.  I believe this matches up well with the existing 
EISA-specific code, which also performs request_region().

	Jeff

Re: [PATCH] Don't call request_region() for 3C90x chips

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>>>> It's generally not a good idea to call request_region() on an 
>>>> address returned by pci_iomap(), even less so on a MMIO address. And 
>>>> there was absolutely no point in claiming the region already claimed 
>>>> by the PCI core, especially with the same PCI generic owner's name. 
>>>> As this is the only case of the must_free_region flag being set, 
>>>> this flag may go away as well...

>>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>> I agree you have identified a bug, but this is not a solution.

>>> The current driver bug is that it calls request_region() potentially 
>>> on an MMIO address, but the solution is _not_ to completely avoid 
>>> reserving the resource.

>>    It's not even a MMIO/PIO address anymore after pci_iomap() -- it 
>> either went thru ioremap() or ioport_map() which both change the 
>> mapping from the physical to the virtual address (or some equivalent 
>> of it for I/O ports).

> Yes.  _Obviously_ you must reserve the resource passed to 
> pci_iomap/ioremap, not the cookie returned by such.

    What somewhat puzzled me is the words "Cardbus drivers already allocate 
for us" in the current driver's vortex_probe1(). What extra drivers, and why 
should they call request_region() for us? :-/

>>> The region registered with the PCI core, but _not_ claimed by anyone. 
>>> Someone still needs to either call pci_{request,release}_regions() or 
>>> request_[mem_]region() to indicate that the resource is reserved.

>>    Sigh, it seems I've missed that difference. So, I'll recast...

> IMO it would be easiest to do pci_{request,release}_regions() in the 
> PCI-only code.  I believe this matches up well with the existing 
> EISA-specific code, which also performs request_region().

    Ugh, I've looked at vortex_remove_one() and found another buglet: they're 
trying to reset the chip there... after calling pci_disable_device(). Guess 
whether they really reset anything. I wonder whether it's accpetable for this 
fix to be put in the same patch...

>     Jeff

WBR, Sergei