Date: Wed, 21 Nov 2012 03:43:02 -0500 From: Mark Johnston <markjdb@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: [patch] fix uplcom(4) clear stall logic for PL2303HX Message-ID: <20121121084302.GA4136@oddish> In-Reply-To: <201211200815.19192.hselasky@c2i.net> References: <20121120025722.GA3338@oddish> <201211200815.19192.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Nov 20, 2012 at 08:15:19AM +0100, Hans Petter Selasky wrote: > On Tuesday 20 November 2012 03:57:22 Mark Johnston wrote: > > > > 1. What exactly is the purpose of clearing ENDPOINT_HALT when a > > userspace program attaches to a device? Is it just to make sure that the > > device fw is in some known good state before starting to transmit data? > > The purpose is to ensure that the so-called data toggle is reset to zero. If a > packet is sent using the wrong data-toggle, it will simply get dropped. This > is not so important for Serial devices, but for other device classes it is. > > If a USB device does not support clear-stall, it is not complying with the > basics of the USB standards defined at usb.org, and I think it is not USB > certified either. That seems to be the case unfortunately, i.e. the device is responding with USB_ERROR_STALLED when it receives a clear stall request over the intr or bulk transfers. > > > > > 2. uplcom(4) tries to clear any stalls after an error in its r/w and > > intr callbacks. Is there some way I can trigger an error so that I can > > test and fix that code too? > > > > Does the device choke on clear-stall? > > You can test that simply by adding a sysctl to the code, which you set to make > the code go to the error case upon transfer completion. > > I suggest you look at storage/umass.c and the reset states for an example on > how to make a non-default asynchronous control transfer. > > When you have a patch I can commit it. > It doesn't seem like the vendor-specific commands help at all in clearing errors. I spent some time playing around with the patch at [1], which adds some reset callbacks that submit the vendor commands, but I couldn't get the device to recover - I just get more STALLED errors back. Looking at the Linux driver (pl2303) doesn't reveal anything helpful - I don't really see any kind of error-handling code. At any rate, my original patch below fixes my problems. I don't like the fact that the error handling is broken, but at least everything (seems) to clean itself up nicely in the error case - the driver just disconnects and reattaches. Thanks, -Mark [1] https://www.student.cs.uwaterloo.ca/~m6johnst/patch/uplcom_reset_logic.patch diff --git a/sys/dev/usb/serial/uplcom.c b/sys/dev/usb/serial/uplcom.c index 4549bad..4998c3f 100644 --- a/sys/dev/usb/serial/uplcom.c +++ b/sys/dev/usb/serial/uplcom.c @@ -433,10 +433,19 @@ uplcom_attach(device_t dev) goto detach; } /* clear stall at first run */ - mtx_lock(&sc->sc_mtx); - usbd_xfer_set_stall(sc->sc_xfer[UPLCOM_BULK_DT_WR]); - usbd_xfer_set_stall(sc->sc_xfer[UPLCOM_BULK_DT_RD]); - mtx_unlock(&sc->sc_mtx); + if (sc->sc_chiptype != TYPE_PL2303HX) { + /* HX variants seem to lock up after a clear stall request. */ + mtx_lock(&sc->sc_mtx); + usbd_xfer_set_stall(sc->sc_xfer[UPLCOM_BULK_DT_WR]); + usbd_xfer_set_stall(sc->sc_xfer[UPLCOM_BULK_DT_RD]); + mtx_unlock(&sc->sc_mtx); + } else { + if (uplcom_pl2303_do(sc->sc_udev, UT_WRITE_VENDOR_DEVICE, + UPLCOM_SET_REQUEST, 8, 0, 0) || + uplcom_pl2303_do(sc->sc_udev, UT_WRITE_VENDOR_DEVICE, + UPLCOM_SET_REQUEST, 9, 0, 0)) + goto detach; + } error = ucom_attach(&sc->sc_super_ucom, &sc->sc_ucom, 1, sc, &uplcom_callback, &sc->sc_mtx); @@ -555,9 +564,6 @@ uplcom_pl2303_init(struct usb_device *udev, uint8_t chiptype) if (err) return (EIO); - if (uplcom_pl2303_do(udev, UT_WRITE_VENDOR_DEVICE, UPLCOM_SET_REQUEST, 8, 0, 0) - || uplcom_pl2303_do(udev, UT_WRITE_VENDOR_DEVICE, UPLCOM_SET_REQUEST, 9, 0, 0)) - return (EIO); return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121121084302.GA4136>