Don't call request_region() for 3C90x chips

PATCH In review Sergei Shtylylov 2 participants 5 messages Jul 10, 2006 – Jul 27, 2006

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Sergei Shtylylov

author

2006-07-10 18:26 UTC #0 lore raw

PATCH 1/1 Don't call request_region() for 3C90x chips

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 <[email protected]>

drivers/net/3c59x.c +1 -7

Index: 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);
}

Jeff Garzik

2006-07-19 17:57 UTC #1 lore raw

Quoted 3 lines · 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 <[email protected]>

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

Sergei Shtylyov

author

2006-07-19 19:09 UTC #2 lore raw

Hello.

Quoted 7 lines · Jeff Garzik

Quoted 3 lines
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 <[email protected]>
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).

Quoted 1 line

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

Quoted 1 line

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

Quoted 1 line

Jeff

WBR, Sergei

Jeff Garzik

2006-07-19 19:22 UTC #3 lore raw

Quoted 6 lines · Sergei Shtylyov

Hello.
Quoted 1 line · Jeff Garzik
Quoted 1 line
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...

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Sergei Shtylyov <[email protected]>
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.

Quoted 3 lines

Quoted 1 line
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

Sergei Shtylyov

author

2006-07-27 19:23 UTC #4 lore raw

Hello.

Quoted 4 lines · Jeff Garzik

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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 <[email protected]>

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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? :-/

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
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...

Quoted 1 line

Jeff

WBR, Sergei