Bug 63583 - libpciaccess 0.13.1 FreeBSD Patch to fix xorg-server-1.12.4 (and later)
libpciaccess 0.13.1 FreeBSD Patch to fix xorg-server-1.12.4 (and later)
Product: xorg
Classification: Unclassified
Component: Lib/pciaccess
7.7 (2012.06)
All FreeBSD
: medium normal
Assigned To: Xorg Project Team
Xorg Project Team
: patch
Depends on:
  Show dependency treegraph
Reported: 2013-04-16 04:11 UTC by John Wehle
Modified: 2013-06-18 12:04 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:

Patch (2.75 KB, text/plain)
2013-04-16 04:11 UTC, John Wehle
no flags Details
Patch to add legacy IO routines for FreeBSD (4.26 KB, patch)
2013-04-16 10:09 UTC, Niclas Zeising
no flags Details | Splinter Review
Implement legacy IO routines for FreeBSD (4.26 KB, patch)
2013-04-17 11:04 UTC, Niclas Zeising
no flags Details | Splinter Review
same thing, using bus_space_* (4.53 KB, patch)
2013-06-17 22:41 UTC, Robert Millan
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description John Wehle 2013-04-16 04:11:46 UTC
Created attachment 78051 [details]

On xorg-server-1.11.4 works fine with a Radeon 7850 running with the VESA
driver on FreeBSD 9.1.  Upgrading to xorg-server-1.12.4 causes the X11
server to crash during startup when int10 / VBE is being initialized.

Tracing through the code shows the problem is cause by libpciaccess
missing routines for implementing legacy I/O on FreeBSD.  With the
enclosed patch applied xorg-server-1.12.4 passes a quick smoke test.
This fix has only be tested on an i386 platform.
Comment 1 Niclas Zeising 2013-04-16 10:09:29 UTC
Created attachment 78063 [details] [review]
Patch to add legacy IO routines for FreeBSD
Comment 2 Niclas Zeising 2013-04-16 10:11:14 UTC
My text somehow disappeared when attaching the patch.
This patch is a better more complete solution to the above problem.  It has been in use for quite some time, and is tested on both i386 and amd64 (x86_64) platform, possibly others.
Comment 3 Niclas Zeising 2013-04-17 11:04:09 UTC
Created attachment 78130 [details] [review]
Implement legacy IO routines for FreeBSD

Here is an updated patch, which fixes a typo.
Comment 4 Alan Coopersmith 2013-04-18 04:24:21 UTC
Could you please provide Signed-off-by tags for the authors?


(git format-patch format, with the commit comments and signature tags is
 preferred, instead of raw diffs, but we can make up a comment if needed.)
Comment 5 Mark Kettenis 2013-04-18 12:10:28 UTC
Patch looks ok to me.  Not sure if FreeBSD has PCI_MAGIC_IO_RANGE on any of its
supported architectures yet.

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
Comment 6 Niclas Zeising 2013-04-19 12:50:11 UTC
FreeBSD does not have PCI_MAGIC_IO_RANGE as far as I can tell, but the patch originally comes from the debian/kFreeBSD project, although it has been slightly modified locally by Jung-uk Kim (jkim@FreeBSD.org).  I don't know if this should be added to the copyright notice as well.  I assumed not, since it was not done when jkim committed the changes.
I have send mail to both Jung-un Kim and Robert Millan asking for their review and sign-off.
Comment 7 Jung-uk Kim 2013-04-19 17:22:10 UTC
I don't think my modifications are copyrightable.

Signed-off-by: Jung-uk Kim <jkim@FreeBSD.org>
Comment 8 Chí-Thanh Christopher Nguyễn 2013-05-23 00:56:20 UTC
I ran into this today on Gentoo amd64-fbsd, the patch from attachment 78130 [details] [review] fixes it.
What is the current status of this patch?
Comment 9 Alan Coopersmith 2013-06-08 03:34:26 UTC
Since a revised patch in "git format-patch" style wasn't provided, I have
cobbled together a commit message from the previous bug comments and
pushed to git master:

To ssh://git.freedesktop.org/git/xorg/lib/libpciaccess
   d76fb36..434cd73  master -> master
Comment 10 Robert Millan 2013-06-17 22:38:57 UTC

FWIW, my original patch (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=669062#55
) is

Signed-off-by: Robert Millan <rmh@freebsd.org>

However, I don't understand why the unambigous calls to bus_space_write_* family of functions were replaced by out*().

Please note that using out*() in userland namespace is dangerous, it can lead to undefined I/O behaviour!!

The problem is that different implementations use different argument order. E.g.

>>>>  FreeBSD code: outw(port, data);
>>>>  Glibc code: outw(data, port);

As GNU/kFreeBSD is an hybrid system, it needs to provide both. In different headers of course.

You can also read about the discussion in:


So please, could you use my original patch instead?
Comment 11 Robert Millan 2013-06-17 22:41:17 UTC
Created attachment 80968 [details] [review]
same thing, using bus_space_*
Comment 12 Niclas Zeising 2013-06-18 09:46:43 UTC
> You can also read about the discussion in:
> http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012470.html

I will have to look through this discussion, I was not aware that it existed.

> So please, could you use my original patch instead?

I unfortunately know very little about how libpciaccess works, only that it does work.  Another FreeBSD developer changed your patch to use in*/out* with the motivation that (from the commit log):
"Replace bus_space(9) functions with native CPU instructions.  These KPIs
should never be used in user space.  Also, this simplifies the patch a bit."
I'll ask him or someone else with more kernel knowledge, to take another look.
Comment 13 Mark Kettenis 2013-06-18 12:04:27 UTC
The "9" in bus_space(9) is a giveaway.  This *is* a kernel-only API.  The fact that it works in userland at all is an artifact of the i386/amd64-specific implementation, where bus_space_tag_t is a simple integer.  On other systems, such as sparc64 and mips, it is a more complex type and these interfaces cannot possibly work outside the kernel.

The outb/outb/outl functions are pretty much standard in the various BSDs, and are intended to be used outside the kernel at least on OpenBSD and NetBSD.  The kernel is supposed to use bus_space(9) after all.  Just the header files from where to get them from aren't really standardized (and it seems NetBSD recently changed its mind on where to put them).

I don't see an issue to be solved.  If there is an actual problem on GNU/kFreeBSD, it should provide a proper <machine/cpufunc.h>.