Re: [patch] fix uplcom(4) clear stall logic for PL2303HX

2012-11-23 Thread Hans Petter Selasky
On Thursday 22 November 2012 06:09:19 Mark Johnston wrote:
 On Wed, Nov 21, 2012 at 11:07:20PM +0100, Hans Petter Selasky wrote:
  Here you go:
  
  http://svnweb.freebsd.org/changeset/base/243380
  
  Please test and verify.
  
  --HPS
 
 Yep, it's working. Thanks a lot!
 
 When I was debugging the problem I noticed the UQ_OPEN_CLEARSTALL quirk,
 and then later discovered that it no longer has any effect. Is there any
 reason for keeping it around? Or can it be removed (patch below)?
 
 Thanks,
 -Mark
 

Done:

http://svnweb.freebsd.org/changeset/base/243435

--HPS
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org


Re: [patch] fix uplcom(4) clear stall logic for PL2303HX

2012-11-21 Thread Mark Johnston
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);
 }
 
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org


Re: [patch] fix uplcom(4) clear stall logic for PL2303HX

2012-11-21 Thread Hans Petter Selasky
On Wednesday 21 November 2012 09:43:02 Mark Johnston wrote:
 Mark Johnston

Here you go:

http://svnweb.freebsd.org/changeset/base/243380

Please test and verify.

--HPS
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org


Re: [patch] fix uplcom(4) clear stall logic for PL2303HX

2012-11-21 Thread Mark Johnston
On Wed, Nov 21, 2012 at 11:07:20PM +0100, Hans Petter Selasky wrote:
 Here you go:
 
 http://svnweb.freebsd.org/changeset/base/243380
 
 Please test and verify.
 
 --HPS

Yep, it's working. Thanks a lot!

When I was debugging the problem I noticed the UQ_OPEN_CLEARSTALL quirk,
and then later discovered that it no longer has any effect. Is there any
reason for keeping it around? Or can it be removed (patch below)?

Thanks,
-Mark

diff --git a/share/man/man4/usb_quirk.4 b/share/man/man4/usb_quirk.4
index 65deafa..f017e01 100644
--- a/share/man/man4/usb_quirk.4
+++ b/share/man/man4/usb_quirk.4
@@ -76,8 +76,6 @@ mouse sends an unknown leading byte
 mouse has Z-axis reversed
 .It UQ_NO_STRINGS
 string descriptors are broken
-.It UQ_OPEN_CLEARSTALL
-device needs clear endpoint stall
 .It UQ_POWER_CLAIM
 hub lies about power status
 .It UQ_SPUR_BUT_UP
diff --git a/sys/dev/usb/quirk/usb_quirk.c b/sys/dev/usb/quirk/usb_quirk.c
index fe15a0a..8f35584 100644
--- a/sys/dev/usb/quirk/usb_quirk.c
+++ b/sys/dev/usb/quirk/usb_quirk.c
@@ -508,7 +508,6 @@ static const char *usb_quirk_str[USB_QUIRK_MAX] = {
[UQ_MS_LEADING_BYTE]= UQ_MS_LEADING_BYTE,
[UQ_MS_REVZ]= UQ_MS_REVZ,
[UQ_NO_STRINGS] = UQ_NO_STRINGS,
-   [UQ_OPEN_CLEARSTALL]= UQ_OPEN_CLEARSTALL,
[UQ_POWER_CLAIM]= UQ_POWER_CLAIM,
[UQ_SPUR_BUT_UP]= UQ_SPUR_BUT_UP,
[UQ_SWAP_UNICODE]   = UQ_SWAP_UNICODE,
diff --git a/sys/dev/usb/quirk/usb_quirk.h b/sys/dev/usb/quirk/usb_quirk.h
index 32a60a1..15d5f15 100644
--- a/sys/dev/usb/quirk/usb_quirk.h
+++ b/sys/dev/usb/quirk/usb_quirk.h
@@ -54,7 +54,6 @@ enum {
UQ_MS_LEADING_BYTE, /* mouse sends an unknown leading byte */
UQ_MS_REVZ, /* mouse has Z-axis reversed */
UQ_NO_STRINGS,  /* string descriptors are broken */
-   UQ_OPEN_CLEARSTALL, /* device needs clear endpoint stall */
UQ_POWER_CLAIM, /* hub lies about power status */
UQ_SPUR_BUT_UP, /* spurious mouse button up events */
UQ_SWAP_UNICODE,/* has some Unicode strings swapped */
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org


[patch] fix uplcom(4) clear stall logic for PL2303HX

2012-11-19 Thread Mark Johnston
Hello all,

I recently bought a PL-2303 USB to serial converter (VID 0x067b, DID
0x2303) to use with an ARM board. When I start cu(1) and power on the
board no messages show up, and if I enter any input, cu exits without
printing anything and uplcom detaches and reinitializes itself. No error
messages are printed by the kernel - all I see is that uplcom(4)
disconnects and reattaches.

After some debugging I found that when the USB stack sends a
ENDPOINT_HALT clear to the OUT bulk endpoint, the hw seems to respond
with an endpoint stalled error, and after that, the uplcom callbacks are
called with error set to USB_ERROR_CANCELLED. I looked at the Linux
driver for this device and found the following code in pl2303.c:

if (priv-type != HX) {
usb_clear_halt(serial-dev, port-write_urb-pipe);
usb_clear_halt(serial-dev, port-read_urb-pipe);
} else {
/* reset upstream data pipes */
pl2303_vendor_write(8, 0, serial);
pl2303_vendor_write(9, 0, serial);
}

Unfortunately, I couldn't find any datasheets which indicate what these
vendor-specific commands mean. However I ended up with the patch below,
and now the device works perfectly. :)
In particular, it seems that we don't want to send a clear ENDPOINT_HALT
request to the HX variant of the device, which is what I have.

I noticed that the device still seems to work if I omit the vendor
commands, and being a total USB newbie I thought I'd ask the following
questions:

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?

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?

I'm happy to test alternative fixes, provide debug output, etc., etc.

Thanks,
-Mark

diff --git a/sys/dev/usb/serial/uplcom.c b/sys/dev/usb/serial/uplcom.c
index 4549bad..4d9ac32 100644
--- a/sys/dev/usb/serial/uplcom.c
+++ b/sys/dev/usb/serial/uplcom.c
@@ -433,10 +433,18 @@ 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) {
+   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 +563,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);
 }
 
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org


Re: [patch] fix uplcom(4) clear stall logic for PL2303HX

2012-11-19 Thread Hans Petter Selasky
On Tuesday 20 November 2012 03:57:22 Mark Johnston wrote:
 Hello all,
 
 I recently bought a PL-2303 USB to serial converter (VID 0x067b, DID
 0x2303) to use with an ARM board. When I start cu(1) and power on the
 board no messages show up, and if I enter any input, cu exits without
 printing anything and uplcom detaches and reinitializes itself. No error
 messages are printed by the kernel - all I see is that uplcom(4)
 disconnects and reattaches.
 
 After some debugging I found that when the USB stack sends a
 ENDPOINT_HALT clear to the OUT bulk endpoint, the hw seems to respond
 with an endpoint stalled error, and after that, the uplcom callbacks are
 called with error set to USB_ERROR_CANCELLED. I looked at the Linux
 driver for this device and found the following code in pl2303.c:
 
   if (priv-type != HX) {
   usb_clear_halt(serial-dev, port-write_urb-pipe);
   usb_clear_halt(serial-dev, port-read_urb-pipe);
   } else {
   /* reset upstream data pipes */
   pl2303_vendor_write(8, 0, serial);
   pl2303_vendor_write(9, 0, serial);
   }
 
 Unfortunately, I couldn't find any datasheets which indicate what these
 vendor-specific commands mean. However I ended up with the patch below,
 and now the device works perfectly. :)
 In particular, it seems that we don't want to send a clear ENDPOINT_HALT
 request to the HX variant of the device, which is what I have.
 
 I noticed that the device still seems to work if I omit the vendor
 commands, and being a total USB newbie I thought I'd ask the following
 questions:
 
 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?

Hi,

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.

 
 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.

--HPS
___
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org