On Sat, Dec 11, 2010 at 08:56:38PM +0000, Jacob Meuser wrote:
quoted text
> 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.



quoted text
> 
> 
> > 
> > 
> > > > 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