Re: [PATCH] 2.3.99pre7-1 - From dev->resource[x].start to pci_resource_start(dev, x)

From: Jeff Garzik (jgarzik@mandrakesoft.com)
Date: Tue May 02 2000 - 13:04:27 EST

Francois Romieu wrote:
>
> The occurences of resource.start/len/end are turned into their
> pci_resource_xxx equivalent. I tryed and not mess some arch specific code
> which looked nearly the same (on the sparc and sbus side typically).
> I haven't verified it compiles but aspirin spills are on my side.
> Bziped patch attached (69ko uncompressed). 93 files are modified.

More comments, as I apply parts of the patch to my tree.

** I wouldn't modify arch PCI code to use pci_resource_start.
pci_resource_start is intended for drivers wanting forward and backwards
compatibility, mainly. arch PCI code does "non-driver" things with the
PCI regions and resources, and pci_resource_start may just obscure code
logic, rather than helping code compatibility.

** There is no need to mask dev->resource[].start values against
PCI_BASE_ADDRESS_{IO,MEM}_MASK. This is a leftover from old code. So
the following example patch works, but is not correct:

- unsigned long high_16 = dev->resource[4].start &
PCI_BASE_ADDRESS_IO_MASK;
+ unsigned long high_16 = pci_resource_start(dev, 4) &
PCI_BASE_ADDRESS_IO_MASK;

** You'll also find sneaky code which does stuff like "pciaddr &= 3"
which is unnecessary for the same reasons, just a little less readable.
:)

** I wouldn't update whitespace in code you don't own. Sure we are
encouraged to follow CodingStyle, but to a maintainer it is incredibly
annoying to have whitespace differences to fix up or work around...

** the following is a bug in IDE code, simply made apparent by your
patch. IDE should test resource[].flags not
'dma_base==PCI_BASE_ADDRESS_IO_MASK'

** pci-resource-start was invented precisely to eliminate this sort of
ifdef. You should work with Jes on this one to make sure 2.2.x
compatibility is maintained.

diff -u --recursive --new-file
linux-2.3.99pre7-1.orig/drivers/net/acenic.c
linux-2.3.99pre7-1/drivers/net/acenic.c
--- linux-2.3.99pre7-1.orig/drivers/net/acenic.c Fri Apr 28
20:59:43 2000
+++ linux-2.3.99pre7-1/drivers/net/acenic.c Sat Apr 29 16:23:24 2000
@@ -497,7 +497,7 @@
 #if (LINUX_VERSION_CODE < 0x02030d)
                dev->base_addr = pdev->base_address[0];
 #else
- dev->base_addr = pdev->resource[0].start;
+ dev->base_addr = pci_resource_start(pdev, 0);

#endif

** ug, this driver needs to test IORESOURCE_xxx against the value in
dev->resource[].flags

diff -u --recursive --new-file
linux-2.3.99pre7-1.orig/drivers/net/fc/iph5526.c
linux-2.3.99pre7-1/drivers/net/fc/iph5526.c
--- linux-2.3.99pre7-1.orig/drivers/net/fc/iph5526.c Sat Mar 18
07:13:53 2000
+++ linux-2.3.99pre7-1/drivers/net/fc/iph5526.c Sat Apr 29 16:23:27 2000
@@ -3800,7 +3800,7 @@
                host->hostt->use_new_eh_code = 1;
                host->this_id = tmpt->this_id;
 
- pci_maddr = pdev->resource[0].start;
+ pci_maddr = pci_resource_start(pdev, 0);
                if ( (pdev->resource[0].flags & PCI_BASE_ADDRESS_SPACE)
!= PCI_BASE_ADDRESS_SPACE_MEMORY) {

** references to resource[0].name are ok only if you are careful.
Casting a const char* to a char* seems like an invitation to trouble.
the olympic driver might be a little naughty here... 

-- 
Jeff Garzik              | Nothing cures insomnia like the
Building 1024            | realization that it's time to get up.
MandrakeSoft, Inc.       |        -- random fortune

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

This archive was generated by hypermail 2b29 : Sun May 07 2000 - 21:00:10 EST