Date: Sat, 16 Oct 2010 16:27:53 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Andrew Thompson <thompsa@freebsd.org> Subject: Re: svn commit: r213852 - in head: lib/libusb sys/dev/usb Message-ID: <20101016132753.GU2392@deviant.kiev.zoral.com.ua> In-Reply-To: <201010161330.31549.hselasky@c2i.net> References: <20101016100051.GS2392@deviant.kiev.zoral.com.ua> <201010161212.43749.hselasky@freebsd.org> <201010161330.31549.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--fQBt9K8gdUvY6oNp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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-l= and > > > > 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? > > >=20 > > > For all kinds of structures that are not ABI-invariant, yes. > >=20 >=20 > Hi, >=20 > I've committed a patch to fix the buildworld breakage after feedback from= =20 > Andreas Tobler. >=20 > http://svn.freebsd.org/changeset/base/213920 >=20 > > 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. >=20 > This will also break the ABI on 8-stable and that was the main reason for= =20 > going the other route. However, most applications access USB via libusb, = so=20 > the breakage would probably be minimal. Do you have any opinions here? Sh= ould=20 > we make an exception for the general rule to not change the ABI within a= =20 > stable branch? >=20 > I'm attaching the other approach, which allows both 32 and 64 bit applica= tions=20 > to use USB using the same IOCTL's. >=20 > See thread: [FreeBSD 8/9] [64-bit IOCTL] Using the USB stack from a 32-bi= t=20 > application under a 64-bit kernel. >=20 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, uin= t16_t frIndex) > { > - xfer->ppBuffer[frIndex] =3D buffer; > + xfer->ppBuffer[frIndex] =3D libusb20_u64ptr(buffer); > return; > } > =20 > @@ -386,7 +386,7 @@ > void > libusb20_tr_setup_bulk(struct libusb20_transfer *xfer, void *pBuf, uint3= 2_t length, uint32_t timeout) > { > - xfer->ppBuffer[0] =3D pBuf; > + xfer->ppBuffer[0] =3D libusb20_u64ptr(pBuf); > xfer->pLength[0] =3D length; > xfer->timeout =3D timeout; > xfer->nFrames =3D 1; > @@ -398,7 +398,7 @@ > { > uint16_t len; > =20 > - xfer->ppBuffer[0] =3D psetup; > + xfer->ppBuffer[0] =3D libusb20_u64ptr(psetup); > xfer->pLength[0] =3D 8; /* fixed */ > xfer->timeout =3D timeout; > =20 > @@ -406,7 +406,7 @@ > =20 > if (len !=3D 0) { > xfer->nFrames =3D 2; > - xfer->ppBuffer[1] =3D pBuf; > + xfer->ppBuffer[1] =3D libusb20_u64ptr(pBuf); > xfer->pLength[1] =3D len; > } else { > xfer->nFrames =3D 1; > @@ -417,7 +417,7 @@ > void > libusb20_tr_setup_intr(struct libusb20_transfer *xfer, void *pBuf, uint3= 2_t length, uint32_t timeout) > { > - xfer->ppBuffer[0] =3D pBuf; > + xfer->ppBuffer[0] =3D libusb20_u64ptr(pBuf); > xfer->pLength[0] =3D length; > xfer->timeout =3D timeout; > xfer->nFrames =3D 1; > @@ -431,7 +431,7 @@ > /* should not happen */ > return; > } > - xfer->ppBuffer[frIndex] =3D pBuf; > + xfer->ppBuffer[frIndex] =3D libusb20_u64ptr(pBuf); > xfer->pLength[frIndex] =3D 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_ > =20 > +#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 =3D temp.max_packet_length; > =20 > /* setup buffer and length lists */ > - fsep->ppBuffer =3D xfer->ppBuffer;/* zero copy */ > - fsep->pLength =3D xfer->pLength; /* zero copy */ > + fsep->ppBuffer =3D libusb20_u64ptr(xfer->ppBuffer); /* zero copy */ > + fsep->pLength =3D libusb20_u64ptr(xfer->pLength); /* zero copy */ > =20 > 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)) > =20 > /* function prototypes */ > =20 > @@ -134,7 +135,6 @@ > TUNABLE_INT("hw.usb.ugen.debug", &ugen_debug); > #endif > =20 > - > /* prototypes */ > =20 > 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 =3D USB_ERR_INVAL; > goto complete; > } > - error =3D copyin(fs_ep.ppBuffer, > + > + /* read frame buffer pointer */ > + error =3D 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); > =20 > @@ -1089,7 +1091,8 @@ > =20 > req =3D xfer->frbuffers[0].buffer; > =20 > - error =3D copyin(fs_ep.pLength, > + /* read frame buffer length */ > + error =3D copyin(UGEN_HW_PTR(fs_ep.pLength), > &length, sizeof(length)); > if (error) { > return (error); > @@ -1099,7 +1102,7 @@ > goto complete; > } > if (length !=3D 0) { > - error =3D copyin(uaddr, req, length); > + error =3D copyin(UGEN_HW_PTR(uaddr), req, length); > if (error) { > return (error); > } > @@ -1159,11 +1162,12 @@ > =20 > for (; n !=3D xfer->nframes; n++) { > =20 > - error =3D copyin(fs_ep.pLength + n, > + /* read frame buffer length */ > + error =3D 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); > =20 > if (length > rem) { > @@ -1174,12 +1178,12 @@ > =20 > if (!isread) { > =20 > - /* we need to know the source buffer */ > - error =3D copyin(fs_ep.ppBuffer + n, > + /* read frame buffer pointer */ > + error =3D 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 =3D xfer->frbuffers[0].buffer; > @@ -1193,7 +1197,7 @@ > } > =20 > /* move data */ > - error =3D copyin(uaddr, kaddr, length); > + error =3D 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 @@ > =20 > for (; n !=3D xfer->nframes; n++) { > =20 > - /* get initial length into "temp" */ > - error =3D copyin(fs_ep.pLength + n, > + /* get initial frame buffer length into "temp" */ > + error =3D 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) { > =20 > - /* we need to know the destination buffer */ > - error =3D copyin(fs_ep.ppBuffer + n, > + /* read destination frame buffer pointer */ > + error =3D 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 =3D USB_ADD_BYTES( > @@ -1323,7 +1327,7 @@ > } > =20 > /* move data */ > - error =3D copyout(kaddr, uaddr, length); > + error =3D copyout(kaddr, UGEN_HW_PTR(uaddr), length); > if (error) { > return (error); > } > @@ -1334,12 +1338,11 @@ > */ > offset +=3D temp; > =20 > - /* update length */ > + /* update frame buffer length */ > error =3D copyout(&length, > - fs_ep.pLength + n, sizeof(length)); > - if (error) { > + UGEN_HW_PTR(fs_ep.pLength + (4 * n)), sizeof(length)); > + if (error) > return (error); > - } > } > =20 > 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; --fQBt9K8gdUvY6oNp Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAky5qFkACgkQC3+MBN1Mb4hXawCg8790Uqptejc7vi9PWKbnZ8yJ TXMAoMqH/yxVZaEc2qxlq2q5os790EXs =O9Du -----END PGP SIGNATURE----- --fQBt9K8gdUvY6oNp--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101016132753.GU2392>