> On Sat, Dec 11, 2010 at 08:56:38PM +0000, Jacob Meuser wrote:
> > On Sat, Dec 11, 2010 at 08:35:00PM +0000, Jacob Meuser wrote:
> > > On Sat, Dec 11, 2010 at 08:14:24PM +0100, Mark Kettenis wrote:
> > > > > Date: Wed, 8 Dec 2010 06:07:30 +0000
> > > > > From: Jacob Meuser <jakemsr@sdf.lonestar.org>
> > > > >
> > > > > I recently got a hp officejet 4500. it's a 3-in-1 printer/scanner/fax.
> > > > > printing works great with the hplip packages. scanning doesn't work at
> > > > > all.
> > > > >
> > > > > I tracked the problem to read() failing in usb_bulk_read() in libusb.
> > > > > errno was EINTR. so I made this function just continue when read() got
> > > > > EINTR. I could then scan, but not very reliably.
> > > > >
> > > > > so I went into the kernel, and tracked it to usbdi_util.c:
> > > > >
> > > > > @ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
> > > > >
> > > > > error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
> > > > > splx(s);
> > > > > if (error) {
> > > > > DPRINTF(("usbd_bulk_transfer: tsleep=%d\n", error));
> > > > > usbd_abort_pipe(pipe);
> > > > > return (USBD_INTERRUPTED);
> > > > > }
> > > > >
> > > > > on a hunch, I changed 'PZERO | PCATCH' to 'PWAIT'. then scanning became
> > > > > 100% reliable. even full-page 1200dpi (~525MB of data) worked
> > > > > perfectly.
> > > > >
> > > > > great. but, what if we need to interrupt the transfer. we don't want
> > > > > to hang here.
> > > > >
> > > > > well, this function takes a timeout. so, it's possible to make it
> > > > > return, even if the transfer stalls. but is this used? I looked
> > > > > at the ports that use libusb. setting 0/infinite timeout for bulk
> > > > > transfers is extremely rare, only set in code marked experimental
> > > > > or problematic. in the kernel, there are however some callers that
> > > > > use USBD_NO_TIMEOUT.
> > > > >
> > > > > so, I offer the following. if there's no timeout, behave like we do
> > > > > now and catch signals, and if there's a timeout, ignore signals.
> > > > >
> > > > > I did the same for interrupt transfers because the situation seems
> > > > > to be the same.
> > > > >
> > > > > thoughts?
> > > >
> > > > 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.
> > >
> > > that application would be libusb itself. is a library supposed
> > > to be messing with signals?
> >
> > to be clear, the actual read() is in usb_bulk_read() in libusb.
> > so, the application doesn't really know that there's a read().
> >
> > I tried making libusb continue, but the problem is that when
> > usbd_bulk_transfer catches the signal, it aborts everything in
> > the pipe. restarting from there correctly is complicated,
> > probably not even possible in a sane way.
>
> I looked at all users of libusb. none deal with usb_{bulk,interrupt}_read
> (the libusb "frontend") quitting with EINTR. none set SA_RESTART.
>
> usbd_{bulk,intr}_transfer() (in the usb(4) stack) do not obey
> SA_RESTART. neither do the ugen(4) ends of those functions.
>
> usbd_transfer() does not catch signals. in fact. the only other
> thing in the usb stack that does catch signals is usbread(), which
> is an interface to read messages from the usb event queue. the
> event queue is stored in software. it doesn't handle tsleep()
> returning ERESTART either, btw.
>
> figuring out an absolute max timeout seems fairly straight-forward.
> timeouts are specified in miliseconds. the usb frame rate is 1000 Hz.
> an empty frame signals the end of a transfer. timeout = bytes,
> basically.
>
> now, usbd_intr_transfer() may be a different story. that will block
> (if not set or non-blocking io) until there is data to read. most
> users of this in libusb do set a timeout though. some also restart
> the read if they get ETIMEDOUT.
>
> I think, catching signals is wrong. at least, it's not expected by
> most of it's users and not even handled correctly in the kernel.
>
> further, by not setting a timeout, it leaves the user to figure out
> that something is wrong.
>
>
>
> >
> >
> > >
> > >
> > > > > Index: usbdi_util.c
> > > > > ===================================================================
> > > > > RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
> > > > > retrieving revision 1.25
> > > > > diff -u -p usbdi_util.c
> > > > > --- usbdi_util.c 26 Jun 2008 05:42:19 -0000 1.25
> > > > > +++ usbdi_util.c 8 Dec 2010 04:58:49 -0000
> > > > > @@ -426,7 +426,7 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
> > > > > u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char *lbl)
> > > > > {
> > > > > usbd_status err;
> > > > > - int s, error;
> > > > > + int s, error, pri;
> > > > >
> > > > > usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
> > > > > usbd_bulk_transfer_cb);
> > > > > @@ -437,7 +437,8 @@ usbd_bulk_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
> > > > > splx(s);
> > > > > return (err);
> > > > > }
> > > > > - error = tsleep((caddr_t)xfer, PZERO | PCATCH, lbl, 0);
> > > > > + pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
> > > > > + error = tsleep((caddr_t)xfer, pri, lbl, 0);
> > > > > splx(s);
> > > > > if (error) {
> > > > > DPRINTF(("usbd_bulk_transfer: tsleep=%d\n", error));
> > > > > @@ -467,7 +468,7 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
> > > > > u_int16_t flags, u_int32_t timeout, void *buf, u_int32_t *size, char *lbl)
> > > > > {
> > > > > usbd_status err;
> > > > > - int s, error;
> > > > > + int s, error, pri;
> > > > >
> > > > > usbd_setup_xfer(xfer, pipe, 0, buf, *size, flags, timeout,
> > > > > usbd_intr_transfer_cb);
> > > > > @@ -478,7 +479,8 @@ usbd_intr_transfer(usbd_xfer_handle xfer, usbd_pipe_ha
> > > > > splx(s);
> > > > > return (err);
> > > > > }
> > > > > - error = tsleep(xfer, PZERO | PCATCH, lbl, 0);
> > > > > + pri = timeout == USBD_NO_TIMEOUT ? (PZERO | PCATCH) : PWAIT;
> > > > > + error = tsleep(xfer, pri, lbl, 0);
> > > > > splx(s);
> > > > > if (error) {
> > > > > DPRINTF(("usbd_intr_transfer: tsleep=%d\n", error));
> > >
> > > --
> > >
jakemsr@sdf.lonestar.org
> > > SDF Public Access UNIX System -
http://sdf.lonestar.org
> >
> > --
> >
jakemsr@sdf.lonestar.org
> > SDF Public Access UNIX System -
http://sdf.lonestar.org
>
> --
>
jakemsr@sdf.lonestar.org
> SDF Public Access UNIX System -
http://sdf.lonestar.org
