Bug 63583

Created attachment 78063 [details] [review]
Patch to add legacy IO routines for FreeBSD

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.

Created attachment 78130 [details] [review]
Implement legacy IO routines for FreeBSD

Here is an updated patch, which fixes a typo.

Could you please provide Signed-off-by tags for the authors?

http://www.x.org/wiki/Development/Documentation/SubmittingPatches#Signing_off_and_reviewing

(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.)

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>

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.

I don't think my modifications are copyrightable.

Signed-off-by: Jung-uk Kim <jkim@FreeBSD.org>

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?

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

Hi,

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:

http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012470.html

So please, could you use my original patch instead?

Created attachment 80968 [details] [review]
same thing, using bus_space_*

> 
> 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.

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>.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/lib/libpciaccess/issues/3.