Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5
PATCH 1/1 Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5
Quoted 1 line
From
219005d9522043bc42ddb51d59688959eed0d443Mon Sep 17 00:00:00 2001
From: Lee Jones <[email protected]>
Date: Tue, 13 Jul 2010 15:02:17 +0100
Subject: [PATCH] UBUNTU: [Upstream] Stop ARM boards crashing when CUPS is loaded
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.
--- arch/arm/include/asm/parport.h | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
arch/arm/include/asm/parport.h +5 -2
@@ -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) */
Signature · 1 line
1.7.0.4
(cc linux-arm-kernel)
Quoted 5 lines · On Tue, 13 Jul 2010 15:05:58 +0100 Lee Jones <[email protected]> wrote:
Quoted 1 line
From
219005d9522043bc42ddb51d59688959eed0d443Mon Sep 17 00:00:00 2001From: Lee Jones <[email protected]>
Date: Tue, 13 Jul 2010 15:02:17 +0100
Subject: [PATCH] UBUNTU: [Upstream] Stop ARM boards crashing when CUPS is loaded
Quoted 8 lines
When CUPS loads, it tries to load several drivers that it may need. When one of these drivers, specifically
parport_pcis 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?
Quoted 1 line
arch/arm/include/asm/parport.h | 7 +++++--
This should probably go via the arm tree.
arch/arm/include/asm/parport.h line 12
Quoted 11 lines
arch/arm/include/asm/parport.h +0
-1@@ -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.
arch/arm/include/asm/parport.h line 19
Quoted 8 lines
arch/arm/include/asm/parport.h +5
-1{ -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:
arch/arm/include/asm/parport.h renamed from arch/arm/include/asm/parport.h~parport-prevent-arm-boards-frmo-crashing-when-cups-is-loaded-fix +6 -5
@@ -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 ; }
_
Quoted 9 lines · Andrew Morton
Quoted 8 lines
When CUPS loads, it tries to load several drivers that it may need. When one of these drivers, specifically
parport_pcis 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.
Quoted 5 lines
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?
Quoted 10 lines · Russell King - ARM Linux
Quoted 9 lines · Andrew Morton
Quoted 8 lines
When CUPS loads, it tries to load several drivers that it may need. When one of these drivers, specifically
parport_pcis 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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 2 lines
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. :)
Quoted 4 lines
Why not just avoid selecting and building
parport_pcon these machines?I mean, the Beagleboard doesn't have PCI nor ISA, so why is
parport_pcbeing 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?
Quoted 5 lines · Lee Jones
Quoted 2 lines · Russell King - ARM Linux
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.
Quoted 7 lines
Quoted 4 lines
Why not just avoid selecting and building
parport_pcon these machines?I mean, the Beagleboard doesn't have PCI nor ISA, so why is
parport_pcbeing built for production use?I am happy to make a
Kconfigchange to disallow OMAP builds from building theparport_pcdriver. 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.)
Quoted 6 lines
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.
Quoted 6 lines · Lee Jones
Quoted 6 lines
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.
Quoted 12 lines · Lee Jones
Quoted 6 lines · Lee Jones
Quoted 6 lines
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.
Quoted 5 lines
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
Quoted 8 lines · Lee Jones
Quoted 6 lines
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
Quoted 6 lines
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
Quoted 8 lines · Milton Miller
Quoted 7 lines · Lee Jones
Quoted 6 lines
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.)
Quoted 6 lines
Quoted 1 line
This sounds brilliant - when are you going to start? </kidding>
It has a long time ago ...
drivers/parport/parport_pc.ccallsparport_pc_find_nonpci_ports, which is inasm/parport.h
I'm not entirely sure what you're trying to say here?
How does that help with the platformisation of the driver?
Quoted 9 lines
Quoted 5 lines
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.hwithasm-generic/parport.hwhich already has a check forCONFIG_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?
Quoted 11 lines · On Fri, 16 Jul 2010 about 10:42:23 -0600 Lee Jones wrote:
Quoted 10 lines · Milton Miller
Quoted 8 lines · Lee Jones
Quoted 6 lines
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>
Quoted 8 lines
Quoted 4 lines
It has a long time ago ...
drivers/parport/parport_pc.ccallsparport_pc_find_nonpci_ports, which is inasm/parport.hI'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.
Quoted 21 lines
Quoted 10 lines
Quoted 6 lines
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.hwithasm-generic/parport.hwhich already has a check forCONFIG_ISA, which appears to only be selected on a few ARM platforms?static int
__devinitparport_pc_find_isa_ports(int autoirq, int autodma); static int__devinitparport_pc_find_nonpci_ports(int autoirq, int autodma){#ifdef CONFIG_ISA return parport_pc_find_isa_ports(autoirq, autodma); #else return 0;#endif
}
Quoted 6 lines
That's perfect!
This would work a treat.
Surely this #ifdef should be in all the
parport.hfiles which callparport_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
Quoted 3 lines · Milton Miller
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_datain 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?
Quoted 4 lines · Lee Jones
Quoted 2 lines
Quoted 2 lines
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.
Signature · 2 lines
Martin Michlmayr http://www.cyrius.com/
Quoted 12 lines · Martin Michlmayr
Quoted 5 lines · Lee Jones
Quoted 3 lines
Quoted 3 lines
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_pcmodule on ARM.
That works for you because Debian no longer supports Netwinders 8-(
Woody