how to fix acpi_pci_root_remap_iospace?

how to fix acpi_pci_root_remap_iospace?

[Luck, Tony]

Bjorn,

Back in commit:

  0a70abb38062 ("PCI/ACPI: Support I/O resources when parsing host bridge resources")

we added acpi_pci_root_remap_iospace().  On ia64 this was a no-op because ia64
didn't define PCI_IOBASE, so the entire body of the function was skipped.

But in the current merge window commit:

  0bbf47eab469 ("ia64: use asm-generic/io.h")

ended up defining PCI_IOBASE for us, and now we die horribly
in early boot with:

  kernel BUG at lib/ioremap.c:72!


Is PCI_IOBASE the right thing to check for to decide whether
acpi_pci_root_remap_iospace() needs to do anything?

The ugly fix would be:


diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 7433035ded95..de06377de13b 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -741,7 +741,7 @@ static void acpi_pci_root_validate_resources(struct device *dev,
 static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode,
 			struct resource_entry *entry)
 {
-#ifdef PCI_IOBASE
+#if defined(PCI_IOBASE) && !defined(CONFIG_IA64)
 	struct resource *res = entry->res;
 	resource_size_t cpu_addr = res->start;
 	resource_size_t pci_addr = cpu_addr - entry->offset;

or we can do some other juggling with defines to get the
same outcome.

-Tony

Re: how to fix acpi_pci_root_remap_iospace?

[Arnd Bergmann]

On Thu, Aug 16, 2018 at 10:45 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> Bjorn,
>
> Back in commit:
>
>   0a70abb38062 ("PCI/ACPI: Support I/O resources when parsing host bridge resources")
>
> we added acpi_pci_root_remap_iospace().  On ia64 this was a no-op because ia64
> didn't define PCI_IOBASE, so the entire body of the function was skipped.
>
> But in the current merge window commit:
>
>   0bbf47eab469 ("ia64: use asm-generic/io.h")
>
> ended up defining PCI_IOBASE for us, and now we die horribly
> in early boot with:
>
>   kernel BUG at lib/ioremap.c:72!

Ah, that explains it. I'm sorry for causing you trouble here, and glad
you figured out the cause.

> Is PCI_IOBASE the right thing to check for to decide whether
> acpi_pci_root_remap_iospace() needs to do anything?
>
> The ugly fix would be:

Another way would be to add

 #include <asm-generic/io.h>
+#undef PCI_IOBASE

in your asm/io.h. This is about as ugly as the your version, but
it would be local to ia64 ;-)

      Arnd

Re: how to fix acpi_pci_root_remap_iospace?

[Luck, Tony]

On Thu, Aug 16, 2018 at 11:10:33PM +0200, Arnd Bergmann wrote:
> Another way would be to add
> 
>  #include <asm-generic/io.h>
> +#undef PCI_IOBASE
> 
> in your asm/io.h. This is about as ugly as the your version, but
> it would be local to ia64 ;-)

Third way ...


Is "0" actually the right value for PCI_IOBASE for some platform?

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *)0)
#endif

Or is this just here to make sure that:

static inline u8 inb(unsigned long addr)
{
        u8 val;

        __io_pbr();
        val = __raw_readb(PCI_IOBASE + addr);
        __io_par();
        return val;
}

etc. Do not throw errors?

Should we really just enclose all of inb, inw, inl, ...
inside of:

#ifdef PCI_IOBASE

... all those static functions that use PCI_IOBASE ...

#endif


-Tony

Re: how to fix acpi_pci_root_remap_iospace?

[Arnd Bergmann]

On Fri, Aug 17, 2018 at 1:27 AM Luck, Tony <tony.luck@intel.com> wrote:
>
> On Thu, Aug 16, 2018 at 11:10:33PM +0200, Arnd Bergmann wrote:
> > Another way would be to add
> >
> >  #include <asm-generic/io.h>
> > +#undef PCI_IOBASE
> >
> > in your asm/io.h. This is about as ugly as the your version, but
> > it would be local to ia64 ;-)
>
> Third way ...
>
>
> Is "0" actually the right value for PCI_IOBASE for some platform?
>
> #ifndef PCI_IOBASE
> #define PCI_IOBASE ((void __iomem *)0)
> #endif
>
> Or is this just here to make sure that:
>
> static inline u8 inb(unsigned long addr)
> {
>         u8 val;
>
>         __io_pbr();
>         val = __raw_readb(PCI_IOBASE + addr);
>         __io_par();
>         return val;
> }
>
> etc. Do not throw errors?

Defining it to zero is the traditional approach on some systems, and it's used
for at least two different reasons, both of which I don't particularly like:

- Some (particularly older) targets that have its I/O space mapped
into its linear
  virtual memory define inb() to be effectively an alias for readb() with the
  same numeric arguments. This kind of works in most cases but breaks in
  many corner cases such as
  * user space using /dev/ioport, which now grants access to all of
kernel memory
  * ISA device drivers using fixed 16-bit addresses on inb/outb, which
now points
    into user space memory
  * drivers that get the correct address from a resource but then truncate it by
    storing it in a 16-bit or 32-bit (on 64-bit machines) local variable.

- Some targets don't have any support for I/O space on their PCI bus and just
   want to get things to compile by setting PCI_IOBASE to zero, this still opens
    up some of the same problems as above, but doesn't really help otherwise.

> Should we really just enclose all of inb, inw, inl, ...
> inside of:
>
> #ifdef PCI_IOBASE
>
> ... all those static functions that use PCI_IOBASE ...

This breaks compilation of a couple of important drivers such as serial-8250
which support either I/O or memory space, so it requires some cleanup
first, or the definition of an alternative nop inb/outb family that does not
try to access the bus.

       Arnd

Re: how to fix acpi_pci_root_remap_iospace?

[Boris Brezillon]

On Fri, 17 Aug 2018 10:47:34 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Fri, Aug 17, 2018 at 1:27 AM Luck, Tony <tony.luck@intel.com> wrote:
> >
> > On Thu, Aug 16, 2018 at 11:10:33PM +0200, Arnd Bergmann wrote:  
> > > Another way would be to add
> > >
> > >  #include <asm-generic/io.h>
> > > +#undef PCI_IOBASE
> > >
> > > in your asm/io.h. This is about as ugly as the your version, but
> > > it would be local to ia64 ;-)  
> >
> > Third way ...
> >
> >
> > Is "0" actually the right value for PCI_IOBASE for some platform?
> >
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *)0)
> > #endif
> >
> > Or is this just here to make sure that:
> >
> > static inline u8 inb(unsigned long addr)
> > {
> >         u8 val;
> >
> >         __io_pbr();
> >         val = __raw_readb(PCI_IOBASE + addr);
> >         __io_par();
> >         return val;
> > }
> >
> > etc. Do not throw errors?  
> 
> Defining it to zero is the traditional approach on some systems, and it's used
> for at least two different reasons, both of which I don't particularly like:
> 
> - Some (particularly older) targets that have its I/O space mapped
> into its linear
>   virtual memory define inb() to be effectively an alias for readb() with the
>   same numeric arguments. This kind of works in most cases but breaks in
>   many corner cases such as
>   * user space using /dev/ioport, which now grants access to all of
> kernel memory
>   * ISA device drivers using fixed 16-bit addresses on inb/outb, which
> now points
>     into user space memory
>   * drivers that get the correct address from a resource but then truncate it by
>     storing it in a 16-bit or 32-bit (on 64-bit machines) local variable.
> 
> - Some targets don't have any support for I/O space on their PCI bus and just
>    want to get things to compile by setting PCI_IOBASE to zero, this still opens
>     up some of the same problems as above, but doesn't really help otherwise.
> 
> > Should we really just enclose all of inb, inw, inl, ...
> > inside of:
> >
> > #ifdef PCI_IOBASE
> >
> > ... all those static functions that use PCI_IOBASE ...  
> 
> This breaks compilation of a couple of important drivers such as serial-8250
> which support either I/O or memory space, so it requires some cleanup
> first, or the definition of an alternative nop inb/outb family that does not
> try to access the bus.

Hm, maybe it's just easier to revert the patch since we got rid of
patches adding COMPILE_TEST to drivers which were using read/writesl()
(it turned out ia64 and sparc were not the only archs to not implement
readsx/writesx() variants, and fixing them is not that easy).

RE: how to fix acpi_pci_root_remap_iospace?

[Luck, Tony]

>> - Some targets don't have any support for I/O space on their PCI bus and just
>>    want to get things to compile by setting PCI_IOBASE to zero, this still opens
>>     up some of the same problems as above, but doesn't really help otherwise.

That sounds horrible. Why would you want to have a driver that can't possibly
work on your platform compile cleanly? That's just asking for trouble. Sombody
might load that driver, and ... all the outb/outw/outl calls just corrupt low memory.

> Hm, maybe it's just easier to revert the patch since we got rid of
> patches adding COMPILE_TEST to drivers which were using read/writesl()
> (it turned out ia64 and sparc were not the only archs to not implement
> readsx/writesx() variants, and fixing them is not that easy).

That sounds like a better course of action.

-Tony

Re: how to fix acpi_pci_root_remap_iospace?

[Boris Brezillon]

On Fri, 17 Aug 2018 15:56:23 +0000
"Luck, Tony" <tony.luck@intel.com> wrote:

> >> - Some targets don't have any support for I/O space on their PCI bus and just
> >>    want to get things to compile by setting PCI_IOBASE to zero, this still opens
> >>     up some of the same problems as above, but doesn't really help otherwise.  
> 
> That sounds horrible. Why would you want to have a driver that can't possibly
> work on your platform compile cleanly? That's just asking for trouble. Sombody
> might load that driver, and ... all the outb/outw/outl calls just corrupt low memory.

Well, COMPILE_TEST is here just for that, and it's actually quite
useful to detect potential compilation errors/warnings and make sure
the driver is portable. So, either we decide that readsx/writesx() are
not standard and we create a Kconfig option to reflect when an arch
implements them so that drivers using those funcs can at least be
compile-tested on a few archs, or we fix all archs that do not
implement those functions.

> 
> > Hm, maybe it's just easier to revert the patch since we got rid of
> > patches adding COMPILE_TEST to drivers which were using read/writesl()
> > (it turned out ia64 and sparc were not the only archs to not implement
> > readsx/writesx() variants, and fixing them is not that easy).  
> 
> That sounds like a better course of action.

The solution I propose here is just a way to get that problem fixed
quickly, but I'd still like to have a way to enable the s3c2xx and
orion NAND driver when COMPILE_TEST=y.

Re: how to fix acpi_pci_root_remap_iospace?

[Arnd Bergmann]

On Fri, Aug 17, 2018 at 6:20 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Fri, 17 Aug 2018 15:56:23 +0000
> "Luck, Tony" <tony.luck@intel.com> wrote:
>
> > >> - Some targets don't have any support for I/O space on their PCI bus and just
> > >>    want to get things to compile by setting PCI_IOBASE to zero, this still opens
> > >>     up some of the same problems as above, but doesn't really help otherwise.
> >
> > That sounds horrible. Why would you want to have a driver that can't possibly
> > work on your platform compile cleanly? That's just asking for trouble. Sombody
> > might load that driver, and ... all the outb/outw/outl calls just corrupt low memory.
>
> Well, COMPILE_TEST is here just for that, and it's actually quite
> useful to detect potential compilation errors/warnings and make sure
> the driver is portable. So, either we decide that readsx/writesx() are
> not standard and we create a Kconfig option to reflect when an arch
> implements them so that drivers using those funcs can at least be
> compile-tested on a few archs, or we fix all archs that do not
> implement those functions.

What I meant is not specifically for compile testing, but for drivers
that are used and required on regular platforms and can be used with
either MMIO or PIO mode. serial_8250 is the typical example here,
but there are others as well.

       Arnd

[PATCH] ia64: Fix kernel BUG at lib/ioremap.c:72!

[Tony Luck]

Commit:
  0bbf47eab469 ("ia64: use asm-generic/io.h")

results in a BUG while booting ia64.  This is because
asm-generic/io.h defines PCI_IOBASE, which results in
the function acpi_pci_root_remap_iospace() doing a lot
of unnecessary (and wrong) things.

I'd suggested an #if !CONFIG_IA64 in the functon, but Arnd
suggested keeping the fix inside the arch/ia64 tree.

Fixes: 0bbf47eab469 ("ia64: use asm-generic/io.h")
Suggested-by: Arnd Bergman <arnd@arndb.de>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/include/asm/io.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h
index 6f952171abf9..1e6fef69bb01 100644
--- a/arch/ia64/include/asm/io.h
+++ b/arch/ia64/include/asm/io.h
@@ -454,6 +454,7 @@ extern void memset_io(volatile void __iomem *s, int c, long n);
 #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
 #define xlate_dev_mem_ptr xlate_dev_mem_ptr
 #include <asm-generic/io.h>
+#undef PCI_IOBASE
 
 # endif /* __KERNEL__ */
 
-- 
2.17.1

Re: [PATCH] ia64: Fix kernel BUG at lib/ioremap.c:72!

[Linus Torvalds]

On Mon, Aug 20, 2018 at 9:31 AM Tony Luck <tony.luck@intel.com> wrote:
> I'd suggested an #if !CONFIG_IA64 in the functon, but Arnd
> suggested keeping the fix inside the arch/ia64 tree.
>
> Fixes: 0bbf47eab469 ("ia64: use asm-generic/io.h")

Applied.

            Linus

Re: how to fix acpi_pci_root_remap_iospace?

[Lorenzo Pieralisi]

On Thu, Aug 16, 2018 at 01:45:07PM -0700, Luck, Tony wrote:
> Bjorn,
> 
> Back in commit:
> 
>   0a70abb38062 ("PCI/ACPI: Support I/O resources when parsing host bridge resources")
> 
> we added acpi_pci_root_remap_iospace().  On ia64 this was a no-op because ia64
> didn't define PCI_IOBASE, so the entire body of the function was skipped.
> 
> But in the current merge window commit:
> 
>   0bbf47eab469 ("ia64: use asm-generic/io.h")
> 
> ended up defining PCI_IOBASE for us, and now we die horribly
> in early boot with:
> 
>   kernel BUG at lib/ioremap.c:72!
> 
> 
> Is PCI_IOBASE the right thing to check for to decide whether
> acpi_pci_root_remap_iospace() needs to do anything?
> 
> The ugly fix would be:
> 
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 7433035ded95..de06377de13b 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -741,7 +741,7 @@ static void acpi_pci_root_validate_resources(struct device *dev,
>  static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode,
>  			struct resource_entry *entry)
>  {
> -#ifdef PCI_IOBASE
> +#if defined(PCI_IOBASE) && !defined(CONFIG_IA64)
>  	struct resource *res = entry->res;
>  	resource_size_t cpu_addr = res->start;
>  	resource_size_t pci_addr = cpu_addr - entry->offset;
> 
> or we can do some other juggling with defines to get the
> same outcome.
> 
> -Tony

If that gives too much trouble we could move
acpi_pci_root_remap_iospace() to arch/arm64 too (patch compile tested
below) given that at the moment it is generic in theory, just let me
know what's the best course of action.

Lorenzo

-- >8 --
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 0e2ea1c78542..06e27879087b 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -98,13 +98,46 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
 	return 0;
 }
 
+static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode,
+			struct resource_entry *entry)
+{
+	struct resource *res = entry->res;
+	resource_size_t cpu_addr = res->start;
+	resource_size_t pci_addr = cpu_addr - entry->offset;
+	resource_size_t length = resource_size(res);
+	unsigned long port;
+
+	if (pci_register_io_range(fwnode, cpu_addr, length))
+		goto err;
+
+	port = pci_address_to_pio(cpu_addr);
+	if (port == (unsigned long)-1)
+		goto err;
+
+	res->start = port;
+	res->end = port + length - 1;
+	entry->offset = port - pci_addr;
+
+	if (pci_remap_iospace(res, cpu_addr) < 0)
+		goto err;
+
+	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
+	return;
+err:
+	res->flags |= IORESOURCE_DISABLED;
+}
+
 static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 {
 	struct resource_entry *entry, *tmp;
+	struct acpi_device *device = ci->bridge;
 	int status;
 
 	status = acpi_pci_probe_root_resources(ci);
 	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+		if (entry->res->flags & IORESOURCE_IO)
+			acpi_pci_root_remap_iospace(&device->fwnode, entry);
+
 		if (!(entry->res->flags & IORESOURCE_WINDOW))
 			resource_list_destroy_entry(entry);
 	}
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 7433035ded95..0f9fb87b6e26 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -738,37 +738,6 @@ static void acpi_pci_root_validate_resources(struct device *dev,
 	}
 }
 
-static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode,
-			struct resource_entry *entry)
-{
-#ifdef PCI_IOBASE
-	struct resource *res = entry->res;
-	resource_size_t cpu_addr = res->start;
-	resource_size_t pci_addr = cpu_addr - entry->offset;
-	resource_size_t length = resource_size(res);
-	unsigned long port;
-
-	if (pci_register_io_range(fwnode, cpu_addr, length))
-		goto err;
-
-	port = pci_address_to_pio(cpu_addr);
-	if (port == (unsigned long)-1)
-		goto err;
-
-	res->start = port;
-	res->end = port + length - 1;
-	entry->offset = port - pci_addr;
-
-	if (pci_remap_iospace(res, cpu_addr) < 0)
-		goto err;
-
-	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
-	return;
-err:
-	res->flags |= IORESOURCE_DISABLED;
-#endif
-}
-
 int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
 {
 	int ret;
@@ -789,10 +758,6 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
 			"no IO and memory resources present in _CRS\n");
 	else {
 		resource_list_for_each_entry_safe(entry, tmp, list) {
-			if (entry->res->flags & IORESOURCE_IO)
-				acpi_pci_root_remap_iospace(&device->fwnode,
-						entry);
-
 			if (entry->res->flags & IORESOURCE_DISABLED)
 				resource_list_destroy_entry(entry);
 			else