On Sat, Oct 16, 2010 at 01:30:31PM +0200, Hans Petter Selasky wrote: > On Saturday 16 October 2010 12:12:43 Hans Petter Selasky wrote: > > On Saturday 16 October 2010 12:00:51 Kostik Belousov wrote: > > > > USB has some shared memory structures which are used in both user-land > > > > and kernel, which are not part of IOCTLs. Your approach means that > > > > there are two sets of IOCTL's of all kinds, one for 32-bit and one for > > > > 64-bit? > > > > > > For all kinds of structures that are not ABI-invariant, yes. > > > > Hi, > > I've committed a patch to fix the buildworld breakage after feedback from > Andreas Tobler. > > http://svn.freebsd.org/changeset/base/213920 > > > The approach that was discussed by me and Andrew earlier this year, was to > > use uint64_t instead of "void *" in shared memory structures. The only > > disadvantage is that this will force you to recompile libusb when you > > update the kernel, and so we kind of put that approach aside to keep > > seamless upgrade compatibility. > > This will also break the ABI on 8-stable and that was the main reason for > going the other route. However, most applications access USB via libusb, so > the breakage would probably be minimal. Do you have any opinions here? Should > we make an exception for the general rule to not change the ABI within a > stable branch? > > I'm attaching the other approach, which allows both 32 and 64 bit > applications > to use USB using the same IOCTL's. > > See thread: [FreeBSD 8/9] [64-bit IOCTL] Using the USB stack from a 32-bit > application under a 64-bit kernel. > This is a choice of the poison :).
Ideally, you would switch to the new ABI and keep old ABI shims around
for binary compatibility. In essence, this is equivalent to the proper
32bit compat shims.
> --HPS
> --- src/lib/libusb/libusb20.c 2010-03-08 16:57:53.000000000 0000
> +++ src/lib/libusb/libusb20.c 2010-03-08 16:57:53.000000000 0000
> @@ -320,7 +320,7 @@
> void
> libusb20_tr_set_buffer(struct libusb20_transfer *xfer, void *buffer,
> uint16_t frIndex)
> {
> - xfer->ppBuffer[frIndex] = buffer;
> + xfer->ppBuffer[frIndex] = libusb20_u64ptr(buffer);
> return;
> }
>
> @@ -386,7 +386,7 @@
> void
> libusb20_tr_setup_bulk(struct libusb20_transfer *xfer, void *pBuf, uint32_t
> length, uint32_t timeout)
> {
> - xfer->ppBuffer[0] = pBuf;
> + xfer->ppBuffer[0] = libusb20_u64ptr(pBuf);
> xfer->pLength[0] = length;
> xfer->timeout = timeout;
> xfer->nFrames = 1;
> @@ -398,7 +398,7 @@
> {
> uint16_t len;
>
> - xfer->ppBuffer[0] = psetup;
> + xfer->ppBuffer[0] = libusb20_u64ptr(psetup);
> xfer->pLength[0] = 8; /* fixed */
> xfer->timeout = timeout;
>
> @@ -406,7 +406,7 @@
>
> if (len != 0) {
> xfer->nFrames = 2;
> - xfer->ppBuffer[1] = pBuf;
> + xfer->ppBuffer[1] = libusb20_u64ptr(pBuf);
> xfer->pLength[1] = len;
> } else {
> xfer->nFrames = 1;
> @@ -417,7 +417,7 @@
> void
> libusb20_tr_setup_intr(struct libusb20_transfer *xfer, void *pBuf, uint32_t
> length, uint32_t timeout)
> {
> - xfer->ppBuffer[0] = pBuf;
> + xfer->ppBuffer[0] = libusb20_u64ptr(pBuf);
> xfer->pLength[0] = length;
> xfer->timeout = timeout;
> xfer->nFrames = 1;
> @@ -431,7 +431,7 @@
> /* should not happen */
> return;
> }
> - xfer->ppBuffer[frIndex] = pBuf;
> + xfer->ppBuffer[frIndex] = libusb20_u64ptr(pBuf);
> xfer->pLength[frIndex] = length;
> return;
> }
> --- src/lib/libusb/libusb20_int.h 2010-03-08 16:57:53.000000000 0000
> +++ src/lib/libusb/libusb20_int.h 2010-03-08 16:57:53.000000000 0000
> @@ -31,6 +31,8 @@
> #ifndef _LIBUSB20_INT_H_
> #define _LIBUSB20_INT_H_
>
> +#define libusb20_u64ptr(ptr) ((uint64_t)(uintptr_t)(ptr))
> +
> struct libusb20_device;
> struct libusb20_backend;
> struct libusb20_transfer;
> @@ -148,7 +150,7 @@
> /*
> * Pointer to a list of buffer pointers:
> */
> - void **ppBuffer;
> + uint64_t *ppBuffer;
> /*
> * Pointer to frame lengths, which are updated to actual length
> * after the USB transfer completes:
> --- src/lib/libusb/libusb20_ugen20.c 2010-03-08 16:57:53.000000000 0000
> +++ src/lib/libusb/libusb20_ugen20.c 2010-03-08 16:57:53.000000000 0000
> @@ -763,8 +763,8 @@
> xfer->maxPacketLen = temp.max_packet_length;
>
> /* setup buffer and length lists */
> - fsep->ppBuffer = xfer->ppBuffer;/* zero copy */
> - fsep->pLength = xfer->pLength; /* zero copy */
> + fsep->ppBuffer = libusb20_u64ptr(xfer->ppBuffer); /* zero copy */
> + fsep->pLength = libusb20_u64ptr(xfer->pLength); /* zero copy */
>
> return (0); /* success */
> }
> --- src/sys/dev/usb/usb_generic.c 2010-05-17 04:19:10.000000000 0000
> +++ src/sys/dev/usb/usb_generic.c 2010-05-17 04:19:10.000000000 0000
> @@ -76,6 +76,7 @@
> #define UGEN_BULK_FS_BUFFER_SIZE (64*32) /* bytes */
> #define UGEN_BULK_HS_BUFFER_SIZE (1024*32) /* bytes */
> #define UGEN_HW_FRAMES 50 /* number of milliseconds per
> transfer */
> +#define UGEN_HW_PTR(u64) ((void *)(uintptr_t)(u64))
>
> /* function prototypes */
>
> @@ -134,7 +135,6 @@
> TUNABLE_INT("hw.usb.ugen.debug", &ugen_debug);
> #endif
>
> -
> /* prototypes */
>
> static int
> @@ -1039,7 +1039,7 @@
> struct usb_device_request *req;
> struct usb_xfer *xfer;
> struct usb_fs_endpoint fs_ep;
> - void *uaddr; /* userland pointer */
> + uint64_t uaddr; /* userland pointer */
> void *kaddr;
> usb_frlength_t offset;
> usb_frlength_t rem;
> @@ -1077,11 +1077,13 @@
> xfer->error = USB_ERR_INVAL;
> goto complete;
> }
> - error = copyin(fs_ep.ppBuffer,
> +
> + /* read frame buffer pointer */
> + error = copyin(UGEN_HW_PTR(fs_ep.ppBuffer),
> &uaddr, sizeof(uaddr));
> - if (error) {
> + if (error)
> return (error);
> - }
> +
> /* reset first frame */
> usbd_xfer_set_frame_offset(xfer, 0, 0);
>
> @@ -1089,7 +1091,8 @@
>
> req = xfer->frbuffers[0].buffer;
>
> - error = copyin(fs_ep.pLength,
> + /* read frame buffer length */
> + error = copyin(UGEN_HW_PTR(fs_ep.pLength),
> &length, sizeof(length));
> if (error) {
> return (error);
> @@ -1099,7 +1102,7 @@
> goto complete;
> }
> if (length != 0) {
> - error = copyin(uaddr, req, length);
> + error = copyin(UGEN_HW_PTR(uaddr), req, length);
> if (error) {
> return (error);
> }
> @@ -1159,11 +1162,12 @@
>
> for (; n != xfer->nframes; n++) {
>
> - error = copyin(fs_ep.pLength + n,
> + /* read frame buffer length */
> + error = copyin(UGEN_HW_PTR(fs_ep.pLength + (4 * n)),
> &length, sizeof(length));
> - if (error) {
> + if (error)
> break;
> - }
> +
> usbd_xfer_set_frame_len(xfer, n, length);
>
> if (length > rem) {
> @@ -1174,12 +1178,12 @@
>
> if (!isread) {
>
> - /* we need to know the source buffer */
> - error = copyin(fs_ep.ppBuffer + n,
> + /* read frame buffer pointer */
> + error = copyin(UGEN_HW_PTR(fs_ep.ppBuffer + (8 * n)),
> &uaddr, sizeof(uaddr));
> - if (error) {
> + if (error)
> break;
> - }
> +
> if (xfer->flags_int.isochronous_xfr) {
> /* get kernel buffer address */
> kaddr = xfer->frbuffers[0].buffer;
> @@ -1193,7 +1197,7 @@
> }
>
> /* move data */
> - error = copyin(uaddr, kaddr, length);
> + error = copyin(UGEN_HW_PTR(uaddr), kaddr, length);
> if (error) {
> break;
> }
> @@ -1216,7 +1220,7 @@
> struct usb_xfer *xfer;
> struct usb_fs_endpoint fs_ep;
> struct usb_fs_endpoint *fs_ep_uptr; /* userland ptr */
> - void *uaddr; /* userland ptr */
> + uint64_t uaddr; /* userland ptr */
> void *kaddr;
> usb_frlength_t offset;
> usb_frlength_t rem;
> @@ -1281,12 +1285,12 @@
>
> for (; n != xfer->nframes; n++) {
>
> - /* get initial length into "temp" */
> - error = copyin(fs_ep.pLength + n,
> + /* get initial frame buffer length into "temp" */
> + error = copyin(UGEN_HW_PTR(fs_ep.pLength + (4 * n)),
> &temp, sizeof(temp));
> - if (error) {
> + if (error)
> return (error);
> - }
> +
> if (temp > rem) {
> /* the userland length has been corrupted */
> DPRINTF("corrupt userland length "
> @@ -1307,12 +1311,12 @@
> }
> if (isread) {
>
> - /* we need to know the destination buffer */
> - error = copyin(fs_ep.ppBuffer + n,
> + /* read destination frame buffer pointer */
> + error = copyin(UGEN_HW_PTR(fs_ep.ppBuffer + (8 * n)),
> &uaddr, sizeof(uaddr));
> - if (error) {
> + if (error)
> return (error);
> - }
> +
> if (xfer->flags_int.isochronous_xfr) {
> /* only one frame buffer */
> kaddr = USB_ADD_BYTES(
> @@ -1323,7 +1327,7 @@
> }
>
> /* move data */
> - error = copyout(kaddr, uaddr, length);
> + error = copyout(kaddr, UGEN_HW_PTR(uaddr), length);
> if (error) {
> return (error);
> }
> @@ -1334,12 +1338,11 @@
> */
> offset += temp;
>
> - /* update length */
> + /* update frame buffer length */
> error = copyout(&length,
> - fs_ep.pLength + n, sizeof(length));
> - if (error) {
> + UGEN_HW_PTR(fs_ep.pLength + (4 * n)), sizeof(length));
> + if (error)
> return (error);
> - }
> }
>
> complete:
> --- src/sys/dev/usb/usb_ioctl.h 2010-02-14 12:03:51.000000000 0000
> +++ src/sys/dev/usb/usb_ioctl.h 2010-02-14 12:03:51.000000000 0000
> @@ -131,9 +131,10 @@
> * NOTE: isochronous USB transfer only use one buffer, but can have
> * multiple frame lengths !
> */
> - void **ppBuffer; /* pointer to userland buffers */
> - uint32_t *pLength; /* pointer to frame lengths, updated
> - * to actual length */
> + uint64_t ppBuffer; /* pointer to 64-bit userland buffer
> pointers */
> + uint64_t pLength; /* pointer to 32-bit frame lengths,
> which
> + * get updated to the actual length
> after
> + * the transfer is complete. */
> uint32_t nFrames; /* number of frames */
> uint32_t aFrames; /* actual number of frames */
> uint16_t flags;
pgpY5IOxxSS5j.pgp
Description: PGP signature
