On Sat, Dec 11, 2010 at 08:14:24PM +0100, Mark Kettenis wrote: > Sorry, but this very much feels like you're hacking around a bug in > the application you're using. If it installs signal handler without > specifying the SA_RESTART flag, it has to deal with read(2) failing > with EINTR.
after talking with ratchov and deraadt, I am convinced the bug is that we have a read() interface that can be interrupted but not restarted reliably. i.e. even if the application deals with EINTR, it's not reliable because data is lost in the kernel. so I took a shot at making ugen's read() interface restartable. diff is below. unfortunately it only works about 90% of the time. the original diff I sent works 100%. this diff is also a bit complicated, and still requires complicated userland code to deal with EINTR, because of timeouts. say you have a 4 second timeout, and get interrupted after 3 seconds. so you restart with a 4 second timeout, then get interrupted again, so you restart ... ad infinitum. otoh, you can gettimeofday and keep track of how long you've been waiting, timersub and reset the timeout ... but then you're potentially using a smaller timeout than is required. this timeout mess is exactly where the 10% failure rate comes from. considering that a) I can't find any code that expects EINTR in this case, b) no other transfers in our usb code are interruptable, c) doing the "right" thing is complicated both in the kernel and in userland, I am going to commit the original diff, minus the priority change. I am just posting this here to let interested people know what the situation is. I agree it would be nice/correct if it were possible to interrupt and restart read(), but I also think it's more important to have things actually work reliably. -- jake...@sdf.lonestar.org SDF Public Access UNIX System - http://sdf.lonestar.org Index: ugen.c =================================================================== RCS file: /cvs/src/sys/dev/usb/ugen.c,v retrieving revision 1.62 diff -u -p ugen.c --- ugen.c 24 Sep 2010 08:33:59 -0000 1.62 +++ ugen.c 17 Dec 2010 12:29:23 -0000 @@ -84,6 +84,9 @@ struct ugen_endpoint { u_char *limit; /* end of circular buffer (isoc) */ u_char *cur; /* current read location (isoc) */ u_int32_t timeout; + usbd_xfer_handle bulk_xfer; + int bulk_cb_pending; + usbd_status bulk_cb_status; struct isoreq { struct ugen_endpoint *sce; usbd_xfer_handle xfer; @@ -109,6 +112,7 @@ void ugenintr(usbd_xfer_handle xfer, usbd_private_hand usbd_status status); void ugen_isoc_rintr(usbd_xfer_handle xfer, usbd_private_handle addr, usbd_status status); +void ugen_bulk_read_cb(usbd_xfer_handle, usbd_private_handle, usbd_status); int ugen_do_read(struct ugen_softc *, int, struct uio *, int); int ugen_do_write(struct ugen_softc *, int, struct uio *, int); int ugen_do_ioctl(struct ugen_softc *, int, u_long, @@ -337,10 +341,20 @@ ugenopen(dev_t dev, int flag, int mode, struct proc *p DPRINTFN(5, ("ugenopen: interrupt open done\n")); break; case UE_BULK: + sce->ibuf = malloc(UGEN_BBSIZE, M_USBDEV, M_WAITOK); + sce->cur = sce->fill = sce->ibuf; + sce->limit = sce->ibuf + UGEN_BBSIZE; err = usbd_open_pipe(sce->iface, edesc->bEndpointAddress, 0, &sce->pipeh); - if (err) + if (err) { + free(sce->ibuf, M_USBDEV); return (EIO); + } + sce->bulk_xfer = usbd_alloc_xfer(sc->sc_udev); + if (sce->bulk_xfer == 0) { + free(sce->ibuf, M_USBDEV); + return (ENOMEM); + } break; case UE_ISOCHRONOUS: if (dir == OUT) @@ -445,7 +459,14 @@ ugenclose(dev_t dev, int flag, int mode, struct proc * case UE_ISOCHRONOUS: for (i = 0; i < UGEN_NISOREQS; ++i) usbd_free_xfer(sce->isoreqs[i].xfer); - + break; + case UE_BULK: + while (sce->bulk_cb_pending) + tsleep(&sce->bulk_cb_pending, PZERO, "ugencb", + 0); + if (sce->bulk_xfer != NULL) + usbd_free_xfer(sce->bulk_xfer); + break; default: break; } @@ -465,8 +486,7 @@ int ugen_do_read(struct ugen_softc *sc, int endpt, struct uio *uio, int flag) { struct ugen_endpoint *sce = &sc->sc_endpoints[endpt][IN]; - u_int32_t n, tn; - char buf[UGEN_BBSIZE]; + u_int32_t n; usbd_xfer_handle xfer; usbd_status err; int s; @@ -535,32 +555,68 @@ ugen_do_read(struct ugen_softc *sc, int endpt, struct } break; case UE_BULK: - xfer = usbd_alloc_xfer(sc->sc_udev); - if (xfer == 0) - return (ENOMEM); - while ((n = min(UGEN_BBSIZE, uio->uio_resid)) != 0) { - DPRINTFN(1, ("ugenread: start transfer %d bytes\n",n)); - tn = n; - err = usbd_bulk_transfer( - xfer, sce->pipeh, - sce->state & UGEN_SHORT_OK ? - USBD_SHORT_XFER_OK : 0, - sce->timeout, buf, &tn, "ugenrb"); - if (err) { - if (err == USBD_INTERRUPTED) - error = EINTR; - else if (err == USBD_TIMEOUT) - error = ETIMEDOUT; - else + s = splusb(); + xfer = sce->bulk_xfer; + while (uio->uio_resid > 0) { + + while (sce->bulk_cb_pending > 0) { + if (sce->bulk_cb_pending > 1) { + printf("%s: bulk_cb_pending=%d\n", + __func__, sce->bulk_cb_pending); + } + error = tsleep(&sce->bulk_cb_pending, + PZERO | PCATCH, "ugenrb", + hz * sce->timeout / 1000); + if (sc->sc_dying) error = EIO; + if (error) { + printf("%s: tsleep error=%d\n", + __func__, error); + if (error == EWOULDBLOCK) { + /* timeout, return 0 */ + error = 0; + } + splx(s); + return (error); + } + } + + if (sce->bulk_cb_status != USBD_NORMAL_COMPLETION) { + if (sce->bulk_cb_status == USBD_TIMEOUT) { + /* timeout, return 0 */ + error = 0; + } else + error = EIO; break; } - DPRINTFN(1, ("ugenread: got %d bytes\n", tn)); - error = uiomove(buf, tn, uio); - if (error || tn < n) + + if (sce->fill > sce->cur) { + n = min(sce->fill - sce->cur, uio->uio_resid); + error = uiomove(sce->cur, n, uio); + if (error) + break; + sce->cur += n; + if (sce->cur == sce->fill) + sce->cur = sce->fill = sce->ibuf; + } + if (uio->uio_resid == 0) break; + + n = min(sce->limit - sce->fill, uio->uio_resid); + usbd_setup_xfer(xfer, sce->pipeh, sce, sce->fill, n, + sce->state & UGEN_SHORT_OK ? + USBD_SHORT_XFER_OK : 0, + sce->timeout, ugen_bulk_read_cb); + err = usbd_transfer(xfer); + sce->bulk_cb_pending++; + if (err != USBD_IN_PROGRESS) { + printf("%s: usbd_transfer error=%d\n", + __func__, err); + error = EIO; + break; + } } - usbd_free_xfer(xfer); + splx(s); break; case UE_ISOCHRONOUS: s = splusb(); @@ -786,6 +842,41 @@ ugen_detach(struct device *self, int flags) return (0); } + +void +ugen_bulk_read_cb(usbd_xfer_handle xfer, usbd_private_handle addr, + usbd_status status) +{ + struct ugen_endpoint *sce = addr; + u_int32_t count; + int s; + + usbd_get_xfer_status(xfer, NULL, NULL, &count, &sce->bulk_cb_status); + + if (sce->bulk_cb_status != USBD_NORMAL_COMPLETION) { + printf("%s: status=%d\n", __func__, sce->bulk_cb_status); + if (sce->bulk_cb_status == USBD_STALLED) + usbd_clear_endpoint_stall_async(sce->pipeh); + else if (sce->bulk_cb_status == USBD_CANCELLED) + printf("%s: bulk xfer cancelled\n", __func__); + else if (sce->bulk_cb_status == USBD_TIMEOUT) + printf("%s: bulk xfer timeout\n", __func__); + goto done; + } + + s = splusb(); + sce->fill += count; + if (sce->fill > sce->limit) + panic("%s: ibuf overfill limit=%p fill=%p\n", __func__, + sce->limit, sce->fill); +done: + if (sce->bulk_cb_pending != 1) + printf("%s: bulk_cb_pending=%d\n", sce->bulk_cb_pending); + sce->bulk_cb_pending = 0; + wakeup(&sce->bulk_cb_pending); + splx(s); +} + void ugenintr(usbd_xfer_handle xfer, usbd_private_handle addr, usbd_status status)
