Bugzilla – Bug 197572
X.Org PCI patches stilll left
Last modified: 2006-10-09 19:55:32 UTC
I checked all the about 80 patches of our X.Org package for 10.1. I could drop most patches, since these were already upstream or don't make sense any longer, and integrated about 34 patches patches to our new X.Org 7 packages. 5 PCI patches (some of them are somewhat IA64 specific) are still left. I didn't try to integrate them, since they didn't went in easily and I don't have any clue of the Xserver PCI code at all and therefore don't want to break it completely. The patches were applied in the following order, which might be important to know! Patch334: p_pci-domain.diff Patch350: p_pci-ia64.diff Patch357: p_pci-ce-x.diff Patch359: p_mappciBIOS_complete.diff %ifarch ia64 Patch364: p_pci-legacy-mmap.diff %endif I'll attach them all. Some informations I still could find about them in our 'FILES' file: p_pci-domain.diff [KEEP?] - fixes Xserver crash on SGI Altix machines (#132308) p_pci-ia64.diff [KEEP?] - Mach64 driver bails out on ia64 because it cannot map device memory. It turns out that some bogus and unneeded code attempts to find the root bridge of the device and fails to do so proberly as there this host-to-pci bridge is not existant. This code has been around for years although it completely unclear what it had been intended for. Fixing this by eliminating the bogus code. (eich, Bug #145953) p_pci-ce-x.diff [KEEP?] - fixes PCI bus scanning on CE systems (pci-pci bridges, Bug #147261) p_mappciBIOS_complete.diff [KEEP?] - fixes system hang (IERR on PCI bus) on Dell machines (Bug #151644) p_pci-legacy-mmap.diff [KEEP?] - fixes legacy area mapping on IA64 (Bug #166112)
Created attachment 95362 [details] p_pci-domain.diff
Created attachment 95365 [details] p_pci-ia64.diff
Created attachment 95366 [details] p_pci-ce-x.diff
Created attachment 95367 [details] p_mappciBIOS_complete.diff
Created attachment 95368 [details] p_pci-legacy-mmap.diff
All these patches need to applied to package xorg-x11-server.
Looks like that "p_mappciBIOS_complete.diff" is already upstream. Only the last hunk in hw/xfree86/os-support/linux/lnx_pci.c is handled slighly different.
I've added the patches now to our xorg-x11-server package, but *disabled* them.
Created attachment 96794 [details] Fixed p_pci-domain.diff I have (sort-of) guessed the additions to /sys related device file (link) changes (is26), but it seems to correlate. I also assume that linuxPciHandleBIOS() (new) needs PCI_BUS_NO_DOMAIN() handling. Fixed several potential buffer overflows due to domain handling. xf86GetPciHostConfigFromTag() has no PCI_BUS_NO_DOMAIN() handling, but never had so far. Don't know the potential impact here. Fixed some potential index overflow handling (academic). Completely rewrote lnx_pci.c device scanning. This needs testing badly!
Created attachment 96866 [details] The hunks in bus/Pci.h were reverted.
But the new p_pci-domain.diff still needs some improvement. :-) lnx_pci.c: In function 'xf86OSLinuxGetPciDevs': lnx_pci.c:46: error: expected '{' before '*' token lnx_pci.c:54: error: 'dirent1' undeclared (first use in this function) lnx_pci.c:54: error: (Each undeclared identifier is reported only once lnx_pci.c:54: error: for each function it appears in.) lnx_pci.c:59: error: 'dirent2' undeclared (first use in this function) lnx_pci.c:75: warning: format '%08x' expects type 'unsigned int', but argument 7 has type 'long unsigned int' lnx_pci.c:75: warning: format '%08x' expects type 'unsigned int', but argument 8 has type 'long unsigned int' lnx_pci.c: In function 'xf86GetPciSizeFromOS': lnx_pci.c:160: warning: unused variable 'fn' lnx_pci.c:160: warning: unused variable 'dev' lnx_pci.c: In function 'xf86GetPciOffsetFromOS': lnx_pci.c:196: warning: unused variable 'fn' lnx_pci.c:196: warning: unused variable 'dev' lnx_pci.c: In function 'xf86GetOSOffsetFromPCI': lnx_pci.c:224: warning: unused variable 'fn' lnx_pci.c:224: warning: unused variable 'dev' make[5]: *** [lnx_pci.lo] Error 1
Created attachment 96867 [details] fixed compilation
Still remaining: lnx_pci.c: In function 'xf86OSLinuxGetPciDevs': lnx_pci.c:48: warning: 'i' may be used uninitialized in this function + unsigned int i, num, devfn; [...] + if ( (file = fopen (c, "r")) ) { + i = 0; + while (i < 7 && fgets (c, 0x200, file)) { + if (sscanf (c, PCIADDR_FMT " " PCIADDR_FMT " " PCIADDR_IGNORE_FMT, &begin, &end) == 2) { + tmp->offset[i] = begin; + tmp->size[i] = end - begin; + i++; + } + } + fclose (file); + } + if (i > 0) { + tmp->next = ret; + ret = tmp; + } else + xfree (tmp); Should we better initialize it with "i=0" in the decleration?
Created attachment 96898 [details] Hopefully fixed patch
p_pci-ia64.diff: - applies smoothly and looks ok to me. p_mappciBIOS_complete.diff: > Looks like that "p_mappciBIOS_complete.diff" is already upstream. Only the > last hunk in hw/xfree86/os-support/linux/lnx_pci.c is handled slighly > different. Indeed. This patch is already upstream including the last hunk for lnx_pci.c! Remaining patches for investigation: - p_pci-ce-x.diff (adjust/review) - p_pci-legacy-mmap.diff (review, apply also for non-IA64 platforms?)
Created attachment 97000 [details] Xorg.0.log with PCI domain patch applied. Looks like there's still some room for improvement. :-)
Created attachment 97003 [details] Xorg.0.log without PCI domain patch For comparison the logfile of an Xserver with working PCI scan code. :-)
Created attachment 97027 [details] Updated patch, using different kernel interface This patch uses a slightly different kernel interface (/sys/bus/pci/devices instead of /sys/devices), as the later one seems to have a *big* kernel bug involved. I'm not marking the old patch as obsolete, because it is the way it should be done, but we cannot use it ATM. I already asked for the support level of /sys/bus/pci (whether it's already depricated or what), but ATM this seems to work correctly. BTW - on ia64 /sys/devices seems to work correctly.
Created attachment 97046 [details] Updated p_pci-ce-x.diff patch Nuked unnecessary changes. Updated to match 7.1.
p_pci-legacy-mmap.diff should currently only be applied for ia64. Implications for other platforms are still unclear, and the patch is trivial to maintain.
The patch from comment #18 still has some debugging output enabled (one ErrorF). This should of course be removed in the final version.
Created attachment 97112 [details] Final p_pci-domain patch This patch finally fixes all issues remaining, verfied with the nv driver. The number of available cards wasn't counted, and the pci resource size was off by one. The new system still differs in the data in its internal data structure, as in /sys the pci addresses are the real addresses, while the /proc addresses contain flags (io/mem, 32/64bit, prfetchable) in the lowest 2 or 4 bits for io and memory, repsectively. These flags are saved internally and removed by checking the flags of some external magic address in xf86GetOSOffsetFromPCI. The new system doesn't save these bits, but due to the masking the final results are the same to the caller of xf86GetOSOffsetFromPCI.
Comment on attachment 95367 [details] p_mappciBIOS_complete.diff already included upstream
Thanks a lot, Matthias!
I've tested the new PCI domain patch (+ PCI-PCI bridge patch) on shannon (works fine!) and submitted a new xorg-x11-server package for STABLE. These patches also fixes the PCI scanning on IA64. On this platform we now see different problems (see Bug #197190), but we no longer handle these in this bugreport, since they seem to be unrelated. Finally closing as FIXED. :-)
Reopening - some patches are broken. Attaching new ones.
Created attachment 100972 [details] New final p_pci-domain patch Fixes: - internal domain number of by one (was supposed to be a cleanup, but other code dependet on this semantics) - fixed another long-standing of-by-1 error
Created attachment 100983 [details] Additional patch p_enable-altrix.diff This additional patch enables the build of the altrix detection routines, which have apparently not been included in Xorg 7.1 yet. This patch needs a autoreconf -fi after application.
Please commit :)
Thanks! Will do so.
Created attachment 101011 [details] Again - p_enable-altrix.diff Sorry, missing include directory...
New patches applied now.