[PATCH] ARM: Orion5x - Restore parts of io.h

[PATCH] ARM: Orion5x - Restore parts of io.h

[Andrew Lunn]

Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd remove the orion5x
io.h. Unfortunetely, this is still needed for the definition of
IO_SPACE_LIMIT which overrides the default 64K. All Orion based
systems have 1Mbyte of IO space per PCI[e] bus, and try to
request_resource() this size.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/Kconfig                        |    1 +
 arch/arm/mach-orion5x/include/mach/io.h |   14 ++++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 arch/arm/mach-orion5x/include/mach/io.h

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 84449dd..8fb7e4a 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -588,6 +588,7 @@ config ARCH_ORION5X
 	select PCI
 	select ARCH_REQUIRE_GPIOLIB
 	select GENERIC_CLOCKEVENTS
+	select NEED_MACH_IO_H
 	select PLAT_ORION
 	help
 	  Support for the following Marvell Orion 5x series SoCs:
diff --git a/arch/arm/mach-orion5x/include/mach/io.h b/arch/arm/mach-orion5x/include/mach/io.h
new file mode 100644
index 0000000..f017fe2
--- /dev/null
+++ b/arch/arm/mach-orion5x/include/mach/io.h
@@ -0,0 +1,14 @@
+/*
+ * arch/arm/mach-orion5x/include/mach/io.h
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __ASM_ARCH_IO_H
+#define __ASM_ARCH_IO_H
+
+#define IO_SPACE_LIMIT		0xffffffff
+#define __io(a)			__typesafe_io(a)
+#endif
-- 
1.7.10

[PATCH] ARM: Orion5x - Restore parts of io.h

[Sergei Shtylyov]

Hello.

On 20-06-2012 10:06, Andrew Lunn wrote:

> Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd

    Please also specify that commit's summary in parens.

> remove the orion5x
> io.h. Unfortunetely, this is still needed for the definition of
> IO_SPACE_LIMIT which overrides the default 64K. All Orion based
> systems have 1Mbyte of IO space per PCI[e] bus, and try to
> request_resource() this size.

> Signed-off-by: Andrew Lunn<andrew@lunn.ch>

WBR, Sergei

[PATCH] ARM: Orion5x - Restore parts of io.h

[Arnd Bergmann]

On Wednesday 20 June 2012, Andrew Lunn wrote:
> Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd remove the orion5x
> io.h. Unfortunetely, this is still needed for the definition of
> IO_SPACE_LIMIT which overrides the default 64K. All Orion based
> systems have 1Mbyte of IO space per PCI[e] bus, and try to
> request_resource() this size.

> diff --git a/arch/arm/mach-orion5x/include/mach/io.h b/arch/arm/mach-orion5x/include/mach/io.h
> new file mode 100644
> index 0000000..f017fe2
> --- /dev/null
> +++ b/arch/arm/mach-orion5x/include/mach/io.h
> @@ -0,0 +1,14 @@
> +/*
> + * arch/arm/mach-orion5x/include/mach/io.h
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __ASM_ARCH_IO_H
> +#define __ASM_ARCH_IO_H
> +
> +#define IO_SPACE_LIMIT		0xffffffff
> +#define __io(a)			__typesafe_io(a)
> +#endif

So if you need 1 MB per bus, why do you make the limit 4GB? Also,
the __io function does not actually point to the IO window at all,
which also appears to be horribly wrong.

My guess is that you actually want this to be

#define IO_SPACE_LIMIT SZ_2MB
#define __io(a)	((void __iomem *)ORION5X_PCI_IO_VIRT_BASE + a)

Your patch otherwise would make the kernel build again, but has
no chance of doing the right thing.

	Arnd

[PATCH] ARM: Orion5x - Restore parts of io.h

[Andrew Lunn]

On Wed, Jun 20, 2012 at 03:14:37PM +0000, Arnd Bergmann wrote:
> On Wednesday 20 June 2012, Andrew Lunn wrote:
> > Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd remove the orion5x
> > io.h. Unfortunetely, this is still needed for the definition of
> > IO_SPACE_LIMIT which overrides the default 64K. All Orion based
> > systems have 1Mbyte of IO space per PCI[e] bus, and try to
> > request_resource() this size.
> 
> > diff --git a/arch/arm/mach-orion5x/include/mach/io.h b/arch/arm/mach-orion5x/include/mach/io.h
> > new file mode 100644
> > index 0000000..f017fe2
> > --- /dev/null
> > +++ b/arch/arm/mach-orion5x/include/mach/io.h
> > @@ -0,0 +1,14 @@
> > +/*
> > + * arch/arm/mach-orion5x/include/mach/io.h
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2.  This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#ifndef __ASM_ARCH_IO_H
> > +#define __ASM_ARCH_IO_H
> > +
> > +#define IO_SPACE_LIMIT		0xffffffff
> > +#define __io(a)			__typesafe_io(a)
> > +#endif
> 
> So if you need 1 MB per bus, why do you make the limit 4GB? Also,
> the __io function does not actually point to the IO window at all,
> which also appears to be horribly wrong.
> 
> My guess is that you actually want this to be
> 
> #define IO_SPACE_LIMIT SZ_2MB
> #define __io(a)	((void __iomem *)ORION5X_PCI_IO_VIRT_BASE + a)
> 
> Your patch otherwise would make the kernel build again, but has
> no chance of doing the right thing.

My patch simply puts back what was removed. Please see:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4d5fc58dbe34b78157c05b319669bb3e064ba8bd#patch20

It was probably broken before. It is probably broken now. It probably
never did the right thing. However, we don't have any hardware to test
with and we think it is probably never used in real life.

Is it worth doing more than putting back the original code?

   Andrew

[PATCH] ARM: Orion5x - Restore parts of io.h

[Arnd Bergmann]

On Wednesday 20 June 2012, Andrew Lunn wrote:
> > 
> > So if you need 1 MB per bus, why do you make the limit 4GB? Also,
> > the __io function does not actually point to the IO window at all,
> > which also appears to be horribly wrong.
> > 
> > My guess is that you actually want this to be
> > 
> > #define IO_SPACE_LIMIT SZ_2MB
> > #define __io(a)       ((void __iomem *)ORION5X_PCI_IO_VIRT_BASE + a)
> > 
> > Your patch otherwise would make the kernel build again, but has
> > no chance of doing the right thing.
> 
> My patch simply puts back what was removed. Please see:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4d5fc58dbe34b78157c05b319669bb3e064ba8bd#patch20
> 
> It was probably broken before. It is probably broken now. It probably
> never did the right thing. However, we don't have any hardware to test
> with and we think it is probably never used in real life.
> 
> Is it worth doing more than putting back the original code?

Well, the broken version has significant side-effects, e.g. you cannot
access /dev/port or load a device driver that pokes at the PCI
I/O space without crashing the kernel. I think it's better to put
something in place that has a chance of working than something that
is known to be broken.

That said, we probably should not have done the change that broke
the kernel further than it was broken already: The assumption back
then was that we'd only remove the files that are known to be
incorrect, be we did not consider the side-effects.

Now that we're broken already, we have the choice between either
putting it back to the state it was in before, or trying to fix
it for real. Right now, I'm leaning towards fixing it because
there is still some time before the 3.5 release. If you can try
the version that I suggested above and verify that it fixes the
problem that was introduced in 4d5fc58dbe3, I'd prefer merging
that.

	Arnd