[PATCH] aic7xxx: fix MMIO for PPC 44x platforms

[PATCH] aic7xxx: fix MMIO for PPC 44x platforms

[Sergei Shtylyov]

The driver stores the the PCI resource address into 'u_long' variable before
calling ioremap_nocache() on it. This warrants kernel oops when the registers
are accessed on PPC 44x platforms which (being 32-bit) have PCI memory space
mapped beyond 4 GB.

The arch/ppc/ kernel has a fixup in ioremap() that creates an illusion of the
PCI I/O and memory resources are mapped below 4 GB, but arch/powerpc/ code got
rid of this trick, having instead CONFIG_RESOURCES_64BIT enabled.

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

---
I had to also change ahc_linux_pci_reserve_io_region() prototype because the
same 'base' variable is passed to it and ahc_linux_pci_reserve_mem_region(),
although 'u_long' is good for the I/O space addressed.

This is the same issue as the one that has been recently addressed by commits
3c34ac36ac1084e571ef9b6fb1d6a5b10ccc1fd0 (e1000: Fix for 32 bits platforms with
64 bits resources) and c976816b6e901341ec3c4653147316c15549a1c4 (siimage: fix
kernel oops on PPC 44x).  The patch has only been compile tested though...

 drivers/scsi/aic7xxx/aic7xxx_osm.h     |    2 +-
 drivers/scsi/aic7xxx/aic7xxx_osm_pci.c |   14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/scsi/aic7xxx/aic7xxx_osm.h
===================================================================
--- linux-2.6.orig/drivers/scsi/aic7xxx/aic7xxx_osm.h
+++ linux-2.6/drivers/scsi/aic7xxx/aic7xxx_osm.h
@@ -365,7 +365,7 @@ struct ahc_platform_data {
 #define AHC_LINUX_NOIRQ	((uint32_t)~0)
 	uint32_t		 irq;		/* IRQ for this adapter */
 	uint32_t		 bios_address;
-	uint32_t		 mem_busaddr;	/* Mem Base Addr */
+	resource_size_t 	 mem_busaddr;	/* Mem Base Addr */
 };
 
 /************************** OS Utility Wrappers *******************************/
Index: linux-2.6/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
===================================================================
--- linux-2.6.orig/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ linux-2.6/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -293,7 +293,7 @@ ahc_linux_pci_exit(void)
 }
 
 static int
-ahc_linux_pci_reserve_io_region(struct ahc_softc *ahc, u_long *base)
+ahc_linux_pci_reserve_io_region(struct ahc_softc *ahc, resource_size_t *base)
 {
 	if (aic7xxx_allow_memio == 0)
 		return (ENOMEM);
@@ -308,10 +308,10 @@ ahc_linux_pci_reserve_io_region(struct a
 
 static int
 ahc_linux_pci_reserve_mem_region(struct ahc_softc *ahc,
-				 u_long *bus_addr,
+				 resource_size_t *bus_addr,
 				 uint8_t __iomem **maddr)
 {
-	u_long	start;
+	resource_size_t	start;
 	int	error;
 
 	error = 0;
@@ -336,7 +336,7 @@ int
 ahc_pci_map_registers(struct ahc_softc *ahc)
 {
 	uint32_t command;
-	u_long	 base;
+	resource_size_t	base;
 	uint8_t	__iomem *maddr;
 	int	 error;
 
@@ -374,12 +374,12 @@ ahc_pci_map_registers(struct ahc_softc *
 		} else
 			command |= PCIM_CMD_MEMEN;
 	} else {
-		printf("aic7xxx: PCI%d:%d:%d MEM region 0x%lx "
+		printf("aic7xxx: PCI%d:%d:%d MEM region 0x%llx "
 		       "unavailable. Cannot memory map device.\n",
 		       ahc_get_pci_bus(ahc->dev_softc),
 		       ahc_get_pci_slot(ahc->dev_softc),
 		       ahc_get_pci_function(ahc->dev_softc),
-		       base);
+		       (uint64_t)base);
 	}
 
 	/*
@@ -398,7 +398,7 @@ ahc_pci_map_registers(struct ahc_softc *
 			       ahc_get_pci_bus(ahc->dev_softc),
 			       ahc_get_pci_slot(ahc->dev_softc),
 			       ahc_get_pci_function(ahc->dev_softc),
-			       base);
+			       (u_long)base);
 		}
 	}
 	ahc_pci_write_config(ahc->dev_softc, PCIR_COMMAND, command, 4);

Re: [PATCH] aic7xxx: fix MMIO for PPC 44x platforms

[James Bottomley]

On Wed, 2008-04-09 at 23:09 +0400, Sergei Shtylyov wrote:
> The driver stores the the PCI resource address into 'u_long' variable before
> calling ioremap_nocache() on it. This warrants kernel oops when the registers
> are accessed on PPC 44x platforms which (being 32-bit) have PCI memory space
> mapped beyond 4 GB.
> 
> The arch/ppc/ kernel has a fixup in ioremap() that creates an illusion of the
> PCI I/O and memory resources are mapped below 4 GB, but arch/powerpc/ code got
> rid of this trick, having instead CONFIG_RESOURCES_64BIT enabled.
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

Well, all I can say about the architecture is: yuk!

However, if this is truly a problem, you only fixed half of it:
aic79xx_osm.h and aic79xx_osm_pci.c have the same problems.

Also, your fixes don't look complete.  This statement in
ahc_pci_map_registers(struct ahc_softc *ahc)

			ahc->bsh.ioport = base;

Should be warning because bsh.ioport is still u_long and base should be
u64 (so a truncation warning).  I think the correct fix (unless you also
have a ludicrous port space?) is simply to cast to (u_long).


> -               printf("aic7xxx: PCI%d:%d:%d MEM region 0x%lx "
> +               printf("aic7xxx: PCI%d:%d:%d MEM region 0x%llx "
>                        "unavailable. Cannot memory map device.\n",
>                        ahc_get_pci_bus(ahc->dev_softc),
>                        ahc_get_pci_slot(ahc->dev_softc),
>                        ahc_get_pci_function(ahc->dev_softc),
> -                      base);
> +                      (uint64_t)base);

This isn't quite right: uint64_t is unsigned long on 64 bit platforms,
so it won't match %llx (you need it to be unsigned long long
explicitly).

> @@ -398,7 +398,7 @@ ahc_pci_map_registers(struct ahc_softc *
>                                ahc_get_pci_bus(ahc->dev_softc),
>                                ahc_get_pci_slot(ahc->dev_softc),
>                                ahc_get_pci_function(ahc->dev_softc),
> -                              base);
> +                              (u_long)base);

This also isn't right ... you need to cast it to unsigned long long and
use %llx in the printf.

James

Re: [PATCH] aic7xxx: fix MMIO for PPC 44x platforms

[Sergei Shtylyov]

Hello.

James Bottomley wrote:

>>The driver stores the the PCI resource address into 'u_long' variable before
>>calling ioremap_nocache() on it. This warrants kernel oops when the registers
>>are accessed on PPC 44x platforms which (being 32-bit) have PCI memory space
>>mapped beyond 4 GB.

>>The arch/ppc/ kernel has a fixup in ioremap() that creates an illusion of the
>>PCI I/O and memory resources are mapped below 4 GB, but arch/powerpc/ code got

    Not I/O, just memory, of course. :-]
    Although PCI I/O space is mapped beyond 4 GB...

>>rid of this trick, having instead CONFIG_RESOURCES_64BIT enabled.

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

> Well, all I can say about the architecture is: yuk!

    It's not alone, there's also Alchemy MIPS SOCs -- arch/mips/ still uses 
the ioremap() trick for their PCI space.

> However, if this is truly a problem, you only fixed half of it:

    A nice machine check occurs in the siimage driver on 44x due to the same 
issue.

> aic79xx_osm.h and aic79xx_osm_pci.c have the same problems.

    Yes, I can fix it in a separate patch.

> Also, your fixes don't look complete.  This statement in
> ahc_pci_map_registers(struct ahc_softc *ahc)

> 			ahc->bsh.ioport = base;

> Should be warning because bsh.ioport is still u_long and base should be
> u64 (so a truncation warning).

    No warnings, just silent truncation (as it was before the patch when the 
64-bit 'resource_size_t' result of pci_resource_start() was put into 'u_long' 
variable).  And since when C compilers warn about truncations due to implicit 
type converions? :-O

> I think the correct fix (unless you also
> have a ludicrous port space?) is simply to cast to (u_long).

    There's nothing to fix here I think. But if you insist I may add an 
explicit type cast...

>>-               printf("aic7xxx: PCI%d:%d:%d MEM region 0x%lx "
>>+               printf("aic7xxx: PCI%d:%d:%d MEM region 0x%llx "
>>                       "unavailable. Cannot memory map device.\n",
>>                       ahc_get_pci_bus(ahc->dev_softc),
>>                       ahc_get_pci_slot(ahc->dev_softc),
>>                       ahc_get_pci_function(ahc->dev_softc),
>>-                      base);
>>+                      (uint64_t)base);

> This isn't quite right: uint64_t is unsigned long on 64 bit platforms,

    However, 'unsigned long long' and 'unsigned long' are both 64-bit there.

> so it won't match %llx (you need it to be unsigned long long
> explicitly).

    OK.

>>@@ -398,7 +398,7 @@ ahc_pci_map_registers(struct ahc_softc *
>>                               ahc_get_pci_bus(ahc->dev_softc),
>>                               ahc_get_pci_slot(ahc->dev_softc),
>>                               ahc_get_pci_function(ahc->dev_softc),
>>-                              base);
>>+                              (u_long)base);

> This also isn't right ... you need to cast it to unsigned long long and
> use %llx in the printf.

    I can do that if you insist but truncation to u_long is safe for I/O ports...

> James

MBR, Sergei