[PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Lee Jones]

>From 219005d9522043bc42ddb51d59688959eed0d443 Mon Sep 17 00:00:00 2001
From: Lee Jones <lee.jones@canonical.com>
Date: Tue, 13 Jul 2010 15:02:17 +0100
Subject: [PATCH] UBUNTU: [Upstream] Stop ARM boards crashing when CUPS is loaded

BugLink: http://bugs.launchpad.net/bugs/601226

When CUPS loads, it tries to load several drivers that it may need.
When one of these drivers, specifically parport_pc is loaded on ARM
based systems, it causes a segmentation fault as the ISA addresses
which are attempted are not writable on non-PC based architectures.
This code prevents ISA addresses from being attempted except on x86.

Signed-off-by: Lee Jones <lee.jones@canonical.com>
---
 arch/arm/include/asm/parport.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/parport.h b/arch/arm/include/asm/parport.h
index 26e94b0..a1874f8 100644
--- a/arch/arm/include/asm/parport.h
+++ b/arch/arm/include/asm/parport.h
@@ -9,10 +9,13 @@
 #ifndef __ASMARM_PARPORT_H
 #define __ASMARM_PARPORT_H
 
-static int __devinit parport_pc_find_isa_ports (int autoirq, int autodma);
 static int __devinit parport_pc_find_nonpci_ports (int autoirq, int autodma)
 {
-	return parport_pc_find_isa_ports (autoirq, autodma);
+/* parport_pc_find_isa_ports uses direct register addresses which are
+ * only correct on x86 architectures. This may have undesirable
+ * consequences (including segfaults) when used on other architectures.
+ */
+	return 0;
 }
 
 #endif /* !(_ASMARM_PARPORT_H) */
-- 
1.7.0.4

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Andrew Morton]

(cc linux-arm-kernel)

On Tue, 13 Jul 2010 15:05:58 +0100
Lee Jones <lee.jones@canonical.com> wrote:

> >From 219005d9522043bc42ddb51d59688959eed0d443 Mon Sep 17 00:00:00 2001
> From: Lee Jones <lee.jones@canonical.com>
> Date: Tue, 13 Jul 2010 15:02:17 +0100
> Subject: [PATCH] UBUNTU: [Upstream] Stop ARM boards crashing when CUPS is loaded
> 
> BugLink: http://bugs.launchpad.net/bugs/601226
> 
> When CUPS loads, it tries to load several drivers that it may need.
> When one of these drivers, specifically parport_pc is loaded on ARM
> based systems, it causes a segmentation fault as the ISA addresses
> which are attempted are not writable on non-PC based architectures.
> This code prevents ISA addresses from being attempted except on x86.
> 

That sounds like a pretty serious problem.  But presumably it isn't -
otherwise it would have been fixed earlier!

So what actions are required to trigger this bug and why aren't others
seeing it?

>  arch/arm/include/asm/parport.h |    7 +++++--

This should probably go via the arm tree.

> 
> diff --git a/arch/arm/include/asm/parport.h b/arch/arm/include/asm/parport.h
> index 26e94b0..a1874f8 100644
> --- a/arch/arm/include/asm/parport.h
> +++ b/arch/arm/include/asm/parport.h
> @@ -9,10 +9,13 @@
>  #ifndef __ASMARM_PARPORT_H
>  #define __ASMARM_PARPORT_H
>  
> -static int __devinit parport_pc_find_isa_ports (int autoirq, int autodma);
>  static int __devinit parport_pc_find_nonpci_ports (int autoirq, int autodma)

It's strange that this function isn't marked inline.  It's in a .h
file.  But many arch/*/include/asm/parport.h's do it this way.  They're
probably broken.  It adds a risk that unneeded code will be generated
into each compilation unit which includes these headers, although gcc
can probably fix that, depending on the version.  Plus it's plain odd.

>  {
> -	return parport_pc_find_isa_ports (autoirq, autodma);
> +/* parport_pc_find_isa_ports uses direct register addresses which are
> + * only correct on x86 architectures. This may have undesirable
> + * consequences (including segfaults) when used on other architectures.
> + */
> +	return 0;
>  }

That comment layout is whacky.  I did this:

--- a/arch/arm/include/asm/parport.h~parport-prevent-arm-boards-frmo-crashing-when-cups-is-loaded-fix
+++ a/arch/arm/include/asm/parport.h
@@ -9,12 +9,13 @@
 #ifndef __ASMARM_PARPORT_H
 #define __ASMARM_PARPORT_H
 
-static int __devinit parport_pc_find_nonpci_ports (int autoirq, int autodma)
+static int __devinit parport_pc_find_nonpci_ports(int autoirq, int autodma)
 {
-/* parport_pc_find_isa_ports uses direct register addresses which are
- * only correct on x86 architectures. This may have undesirable
- * consequences (including segfaults) when used on other architectures.
- */
+	/*
+	 * parport_pc_find_isa_ports() uses direct register addresses which are
+	 * only correct on x86 architectures. This may have undesirable
+	 * consequences (including segfaults) when used on other architectures.
+	 */
 	return 0;
 }
 
_

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Russell King - ARM Linux]

On Thu, Jul 15, 2010 at 01:02:14PM -0700, Andrew Morton wrote:
> > BugLink: http://bugs.launchpad.net/bugs/601226
> > 
> > When CUPS loads, it tries to load several drivers that it may need.
> > When one of these drivers, specifically parport_pc is loaded on ARM
> > based systems, it causes a segmentation fault as the ISA addresses
> > which are attempted are not writable on non-PC based architectures.
> > This code prevents ISA addresses from being attempted except on x86.
> > 
> 
> That sounds like a pretty serious problem.  But presumably it isn't -
> otherwise it would have been fixed earlier!
> 
> So what actions are required to trigger this bug and why aren't others
> seeing it?

Note that we have machines which have ISA parallel ports, so it's not
this simple.

Why not just avoid selecting and building parport_pc on these machines?

I mean, the Beagleboard doesn't have PCI nor ISA, so why is parport_pc
being built for production use?

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Lee Jones]

On 15/07/10 21:06, Russell King - ARM Linux wrote:
> On Thu, Jul 15, 2010 at 01:02:14PM -0700, Andrew Morton wrote:
>>> BugLink: http://bugs.launchpad.net/bugs/601226
>>>
>>> When CUPS loads, it tries to load several drivers that it may need.
>>> When one of these drivers, specifically parport_pc is loaded on ARM
>>> based systems, it causes a segmentation fault as the ISA addresses
>>> which are attempted are not writable on non-PC based architectures.
>>> This code prevents ISA addresses from being attempted except on x86.
>>>
>>
>> That sounds like a pretty serious problem.  But presumably it isn't -
>> otherwise it would have been fixed earlier!

Well it is a problem on OMAP based boards.

I've personally tested it on OMAP3 and OMAP4 based machines. 

Perhaps the memory is not reserved correctly.

>> So what actions are required to trigger this bug and why aren't others
>> seeing it?

Probably because most other chips either manage to reserve the memory 
successfully, or do not attempt to load the parport_pc driver, either as
a module or built-in.

All I have to do to recreate this bug is load the module or build in the
parport_pc driver.

> Note that we have machines which have ISA parallel ports, so it's not
> this simple.

Do they have ISA parallel ports, or do they just pretend to?

I found this scattered about:

/*
 * We don't actually have real ISA nor PCI buses, but there is so many
 * drivers out there that might just work if we fake them...
 */

Clearly parport_pc isn't one of them. :)

> Why not just avoid selecting and building parport_pc on these machines?

> I mean, the Beagleboard doesn't have PCI nor ISA, so why is parport_pc
> being built for production use?

I am happy to make a Kconfig change to disallow OMAP builds from building
the parport_pc driver. Do you think this would be more sensible?

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Russell King - ARM Linux]

On Fri, Jul 16, 2010 at 10:08:26AM +0100, Lee Jones wrote:
> On 15/07/10 21:06, Russell King - ARM Linux wrote:
> > Note that we have machines which have ISA parallel ports, so it's not
> > this simple.
> 
> Do they have ISA parallel ports, or do they just pretend to?

I have four machines here which have real ISA parallel ports - as in
an ISA multi-IO chip with the serial/parallel/ide ports at the standard
PC IO offsets.  Whether there are more or not, I couldn't say.

When platforms implement the PCI/ISA IO macros correctly (as these four
platforms do), then these drivers work with no modification.

The problem comes when determining what is a "correct" implementation
for some machines - particularly machines which have no PCI/ISA busses
but have PCMCIA support.  PCMCIA has its own ISA-like bus, which can
appear to be quite different from PCs - eg, each socket has its own
separate chunk of MMIO emulating the ISA IO accesses to the cards.

In this case, most platforms prefer to have the PCI/ISA IO macros just
dereference the address they're passed, and arrange for the PCMCIA
support to provide a base address relevant to the IO mapping which has
been established.

This leads to drivers which try the standard ISA IO addresses
dereferencing addresses within the first page, thereby causing a kernel
oops.

> > Why not just avoid selecting and building parport_pc on these machines?
> 
> > I mean, the Beagleboard doesn't have PCI nor ISA, so why is parport_pc
> > being built for production use?
> 
> I am happy to make a Kconfig change to disallow OMAP builds from building
> the parport_pc driver. Do you think this would be more sensible?

Making Kconfig changes to disallow drivers which don't work on certain
platforms will result in a massive expansion of dependencies.  I'm
not certain that this is the right solution.

The best solution is probably for the parport code to go through a
modernisation cycle like the serial code did, essentially using
platform devices to pass the base addresses.  This would make the
driver more portable, and eliminates this problem entirely (because
platforms which don't have parports won't register the platform device(s)
necessary for parport to even probe illegal addresses.)

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Lee Jones]

> The best solution is probably for the parport code to go through a
> modernisation cycle like the serial code did, essentially using
> platform devices to pass the base addresses.  This would make the
> driver more portable, and eliminates this problem entirely (because
> platforms which don't have parports won't register the platform device(s)
> necessary for parport to even probe illegal addresses.)

This sounds brilliant - when are you going to start? </kidding>

In all seriousness, do you think anyone is likely to undertake this 
work anytime soon? I am seeing this problem in a distribution which 
is due for release in October. I have no problem implementing a config
change in the meantime, but as you say, a more _correct_ and portable
solution should be sought.

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Lee Jones]

On 16/07/10 10:32, Lee Jones wrote:
>> The best solution is probably for the parport code to go through a
>> modernisation cycle like the serial code did, essentially using
>> platform devices to pass the base addresses.  This would make the
>> driver more portable, and eliminates this problem entirely (because
>> platforms which don't have parports won't register the platform device(s)
>> necessary for parport to even probe illegal addresses.)

I have lobbied for one of our partners to conduct this work, but I don't
think this would be something that is in their interest to fix. 
Nevertheless, I am currently waiting on a reply from them.

FAO GregKH,

Do you think this would be something that may interest you and your Linux 
drivers project? Or perhaps a student who wants to get their feet wet and
play with some platform driver code.

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Greg KH]

On Fri, Jul 16, 2010 at 12:12:07PM +0100, Lee Jones wrote:
> On 16/07/10 10:32, Lee Jones wrote:
> >> The best solution is probably for the parport code to go through a
> >> modernisation cycle like the serial code did, essentially using
> >> platform devices to pass the base addresses.  This would make the
> >> driver more portable, and eliminates this problem entirely (because
> >> platforms which don't have parports won't register the platform device(s)
> >> necessary for parport to even probe illegal addresses.)
> 
> I have lobbied for one of our partners to conduct this work, but I don't
> think this would be something that is in their interest to fix. 
> Nevertheless, I am currently waiting on a reply from them.
> 
> FAO GregKH,
> 
> Do you think this would be something that may interest you and your Linux 
> drivers project? Or perhaps a student who wants to get their feet wet and
> play with some platform driver code.

Yes, sure, that would be good.  Post it on the driverdevel mailing list
and see who responds.

thanks,

greg k-h

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Milton Miller]

On Fri Jul 16 2010 about 05:33:06 EST,  Lee Jones wrote:
> > The best solution is probably for the parport code to go through a
> > modernisation cycle like the serial code did, essentially using
> > platform devices to pass the base addresses. This would make the
> > driver more portable, and eliminates this problem entirely (because
> > platforms which don't have parports won't register the platform device(s)
> > necessary for parport to even probe illegal addresses.)
> 
> This sounds brilliant - when are you going to start? </kidding>

It has a long time ago ...

drivers/parport/parport_pc.c calls parport_pc_find_nonpci_ports,
which is in asm/parport.h

> 
> In all seriousness, do you think anyone is likely to undertake this
> work anytime soon? I am seeing this problem in a distribution which
> is due for release in October. I have no problem implementing a config
> change in the meantime, but as you say, a more _correct_ and portable
> solution should be sought.

Why not replace the arm asm/parport.h with asm-generic/parport.h which
already has a check for CONFIG_ISA, which appears to only be selected
on a few ARM platforms?

milton

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Lee Jones]

On 16/07/10 17:23, Milton Miller wrote:
> On Fri Jul 16 2010 about 05:33:06 EST,  Lee Jones wrote:
>>> The best solution is probably for the parport code to go through a
>>> modernisation cycle like the serial code did, essentially using
>>> platform devices to pass the base addresses. This would make the
>>> driver more portable, and eliminates this problem entirely (because
>>> platforms which don't have parports won't register the platform device(s)
>>> necessary for parport to even probe illegal addresses.)
>>
>> This sounds brilliant - when are you going to start? </kidding>
> 
> It has a long time ago ...
> 
> drivers/parport/parport_pc.c calls parport_pc_find_nonpci_ports,
> which is in asm/parport.h

I'm not entirely sure what you're trying to say here?

How does that help with the platformisation of the driver?

>> In all seriousness, do you think anyone is likely to undertake this
>> work anytime soon? I am seeing this problem in a distribution which
>> is due for release in October. I have no problem implementing a config
>> change in the meantime, but as you say, a more _correct_ and portable
>> solution should be sought.
> 
> Why not replace the arm asm/parport.h with asm-generic/parport.h which
> already has a check for CONFIG_ISA, which appears to only be selected
> on a few ARM platforms?

static int __devinit parport_pc_find_isa_ports(int autoirq, int autodma);
static int __devinit parport_pc_find_nonpci_ports(int autoirq, int autodma)
{
#ifdef CONFIG_ISA
	return parport_pc_find_isa_ports(autoirq, autodma);
#else
	return 0;
#endif
}

That's perfect! 

This would work a treat.

Surely this #ifdef should be in all the parport.h files which call
parport_pc_find_isa_ports?

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Milton Miller]

On Fri, 16 Jul 2010 about 10:42:23 -0600 Lee Jones wrote:
>On 16/07/10 17:23, Milton Miller wrote:
> > On Fri Jul 16 2010 about 05:33:06 EST,  Lee Jones wrote:
> > > > The best solution is probably for the parport code to go through a
> > > > modernisation cycle like the serial code did, essentially using
> > > > platform devices to pass the base addresses. This would make the
> > > > driver more portable, and eliminates this problem entirely (because
> > > > platforms which don't have parports won't register the platform device(s)
> > > > necessary for parport to even probe illegal addresses.)
> > >
> > > This sounds brilliant - when are you going to start? </kidding>
> > 
> > It has a long time ago ...
> > 
> > drivers/parport/parport_pc.c calls parport_pc_find_nonpci_ports,
> > which is in asm/parport.h
>
>I'm not entirely sure what you're trying to say here?
>
>How does that help with the platformisation of the driver?

I was reading quickly, but my point was the code already defers to the
architecture the method of finding the ports to scan, which is the
important part.

I just looked at the 8250 code and it abuses the platform device model.
Instead of a platform device for each port, it has multiple port
descriptions set via platform_data in one or a a few device instances.
It then only uses this information to fill in its internal array of all
ports, which drives the actual registration with the serial core.  It also
registers a platform device to to get suspend and resume hooks, but now has
to scan its list of ports for all the instances driven from this "device".
Yech.  Please don't use this as an example of a modern driver.


>
> > > In all seriousness, do you think anyone is likely to undertake this
> > > work anytime soon? I am seeing this problem in a distribution which
> > > is due for release in October. I have no problem implementing a config
> > > change in the meantime, but as you say, a more _correct_ and portable
> > > solution should be sought.
> > 
> > Why not replace the arm asm/parport.h with asm-generic/parport.h which
> > already has a check for CONFIG_ISA, which appears to only be selected
> > on a few ARM platforms?
>
>static int __devinit parport_pc_find_isa_ports(int autoirq, int autodma);
>static int __devinit parport_pc_find_nonpci_ports(int autoirq, int autodma)
>{
>#ifdef CONFIG_ISA
>	return parport_pc_find_isa_ports(autoirq, autodma);
>#else
>	return 0;
>#endif
>}
>
>That's perfect! 
>
>This would work a treat.
>
>Surely this #ifdef should be in all the parport.h files which call
>parport_pc_find_isa_ports?

No, as CONFIG_ISA is supposed to be ISA slots, and other architectures
may frequently have 8250 ports at the pc legacy port numbers without
ISA slots.

milton

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Russell King - ARM Linux]

On Fri, Jul 16, 2010 at 02:38:38PM -0500, Milton Miller wrote:
> I just looked at the 8250 code and it abuses the platform device model.
> Instead of a platform device for each port, it has multiple port
> descriptions set via platform_data in one or a a few device instances.

I don't agree with that assessment (but then I'm the one who created
that.)  Having one platform device for each port would have been
absurd given the number of ISA addresses that 8250 ports appear at.

Who'd want 64+ platform devices for serial ports?

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Martin Michlmayr]

* Lee Jones <lee.jones@canonical.com> [2010-07-16 10:08]:
> >> That sounds like a pretty serious problem.  But presumably it isn't -
> >> otherwise it would have been fixed earlier!
> 
> Well it is a problem on OMAP based boards.

It's not just OMAP.  Debian received a similar bug report a few days
too and it was on Marvell Orion hardware.  I simply disabled the
parport_pc module on ARM.

-- 
Martin Michlmayr
http://www.cyrius.com/

Re: [PATCH] Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5

[Woody Suwalski]

Martin Michlmayr wrote:
> * Lee Jones<lee.jones@canonical.com>  [2010-07-16 10:08]:
>    
>>>> That sounds like a pretty serious problem.  But presumably it isn't -
>>>> otherwise it would have been fixed earlier!
>>>>          
>> Well it is a problem on OMAP based boards.
>>      
> It's not just OMAP.  Debian received a similar bug report a few days
> too and it was on Marvell Orion hardware.  I simply disabled the
> parport_pc module on ARM.
>
>    
That works for you because Debian no longer supports Netwinders 8-(

Woody