Subject: Re: kern/36276 (ucycom causes kernel panic when accessing Delorme
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: David Howland <dhowland@users.sourceforge.net>
List: netbsd-bugs
Date: 05/17/2007 04:25:02
The following reply was made to PR kern/36276; it has been noted by GNATS.
From: David Howland <dhowland@users.sourceforge.net>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/36276 (ucycom causes kernel panic when accessing Delorme
USB Earthmate LT-20 GPS)
Date: Thu, 17 May 2007 00:23:31 -0400
I have modified the ucycom driver to use a usbd_setup_xfer and a
callback. Specifically, I have reimplemented ucycomstart(). It should
be correct since its mostly copied from ucom. I have tested it with my
Delorme USB Earthmate LT-20 GPS. (this patch also adds support for that
device)
The patch is included. Sorry for all the extra junk in the diff, my
text editor automatically removes trailing spaces from lines. Also, I
have left (2) comments in the file that log the steps I took to update
the file, I assume that those will be removed if my changes are committed.
********************************************************
--- ucycom.c.orig 2007-05-08 14:41:32.000000000 -0400
+++ ucycom.c 2007-05-17 00:08:30.000000000 -0400
@@ -5,7 +5,7 @@
* All rights reserved.
*
* This code is derived from software contributed to The NetBSD Foundation
- * by Nick Hudson
+ * by Nick Hudson
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -157,6 +157,7 @@
Static int ucycomparam(struct tty *, struct termios *);
Static void ucycomstart(struct tty *);
+Static void ucycomwritecb(usbd_xfer_handle, usbd_private_handle,
usbd_status);
Static void ucycom_intr(struct uhidev *, void *, u_int);
Static int ucycom_configure(struct ucycom_softc *, uint32_t, uint8_t);
Static void tiocm_to_ucycom(struct ucycom_softc *, u_long, int);
@@ -175,6 +176,7 @@
Static const struct usb_devno ucycom_devs[] = {
{ USB_VENDOR_CYPRESS, USB_PRODUCT_CYPRESS_USBRS232 },
{ USB_VENDOR_DELORME, USB_PRODUCT_DELORME_EARTHMATE },
+ { USB_VENDOR_DELORME, USB_PRODUCT_DELORME_EARTHMATE_LT20 },
};
#define ucycom_lookup(v, p) usb_lookup(ucycom_devs, v, p)
@@ -319,7 +321,7 @@
sc = ucycom_cd.cd_devs[unit];
DPRINTF(("ucycomopen: sc=%p\n", sc));
-
+
if (sc == NULL)
return (ENXIO);
@@ -363,7 +365,7 @@
SET(t.c_cflag, CRTSCTS);
if (ISSET(sc->sc_swflags, TIOCFLAG_MDMBUF))
SET(t.c_cflag, MDMBUF);
-
+
tp->t_ospeed = 0;
(void) ucycomparam(tp, &t);
tp->t_iflag = TTYDEF_IFLAG;
@@ -374,7 +376,7 @@
/* Allocate an output report buffer */
sc->sc_obuf = malloc(sc->sc_olen, M_USBDEV, M_WAITOK);
-
+
DPRINTF(("ucycomopen: sc->sc_obuf=%p\n", sc->sc_obuf));
#if 0
@@ -448,12 +450,22 @@
return (0);
}
+/*
+ * Modified to use usbd_transfer like ucom rather than uhidev_write
+ * - Remove uhidev_write
+ * - Add usbd_setup_xfer
+ * - 'cnt' becomes 'len'
+ * - Change 'err' from int to usbd_status
+ * - Replace all parameters to usbd_setup_xfer with equivalents from
uhidev_write
+ * - Moved the wrap-up stuff to the callback function
+ */
Static void
ucycomstart(struct tty *tp)
{
struct ucycom_softc *sc = ucycom_cd.cd_devs[UCYCOMUNIT(tp->t_dev)];
+ usbd_status err;
u_char *data;
- int cnt, len, err, s;
+ int cnt, len, s;
if (sc->sc_dying)
return;
@@ -490,7 +502,7 @@
}
SET(tp->t_state, TS_BUSY);
-
+
/*
* The 8 byte output report uses byte 0 for control and byte
* count.
@@ -498,7 +510,7 @@
* The 32 byte output report uses byte 0 for control. Byte 1
* is used for byte count.
*/
- memset(sc->sc_obuf, 0, sc->sc_olen);
+ memset(sc->sc_obuf, 0, sc->sc_olen);
len = cnt;
switch (sc->sc_olen) {
case 8:
@@ -506,7 +518,7 @@
DPRINTF(("ucycomstart(8): big buffer %d chars\n", len));
len = sc->sc_olen - 1;
}
-
+
memcpy(sc->sc_obuf + 1, data, len);
sc->sc_obuf[0] = len | sc->sc_mcr;
@@ -523,13 +535,13 @@
}
#endif
break;
-
+
case 32:
if (cnt > sc->sc_olen - 2) {
DPRINTF(("ucycomstart(32): big buffer %d chars\n", len));
len = sc->sc_olen - 2;
}
-
+
memcpy(sc->sc_obuf + 2, data, len);
sc->sc_obuf[0] = sc->sc_mcr;
sc->sc_obuf[1] = len;
@@ -546,7 +558,7 @@
}
#endif
break;
-
+
default:
DPRINTFN(2,("ucycomstart: unknown output report size (%zd)\n",
sc->sc_olen));
@@ -566,16 +578,79 @@
}
}
#endif
- err = uhidev_write(sc->sc_hdev.sc_parent, sc->sc_obuf, sc->sc_olen);
- if (err) {
- DPRINTF(("ucycomstart: error doing uhidev_write = %d\n", err));
+ DPRINTFN(4,("ucycomstart: %d chars\n", len));
+ usbd_setup_xfer(sc->sc_hdev.sc_parent->sc_oxfer,
+ sc->sc_hdev.sc_parent->sc_opipe,
+ (usbd_private_handle)sc, sc->sc_obuf, sc->sc_olen,
+ USBD_NO_COPY, USBD_NO_TIMEOUT, ucycomwritecb);
+ /* What can we do on error? */
+ err = usbd_transfer(sc->sc_hdev.sc_parent->sc_oxfer);
+#ifdef UCYCOM_DEBUG
+ if (err != USBD_IN_PROGRESS)
+ DPRINTF(("ucycomstart: err=%s\n", usbd_errstr(err)));
+#endif
+
+out:
+ splx(s);
+}
+
+/*
+ * Copied from ucom
+ * - removed call to rnd_add_uint32 (no need to complicate things)
+ * - change 'cc' to 'len', get 'len' by the same method as ucycomstart
+ * - change usbd_clear_endpoint to something similar to
usbd_intr_transfer()
+ * - change ucom_softc to ucycom_softc
+ */
+Static void
+ucycomwritecb(usbd_xfer_handle xfer, usbd_private_handle p, usbd_status
status)
+{
+ struct ucycom_softc *sc = (struct ucycom_softc *)p;
+ struct tty *tp = sc->sc_tty;
+ int cnt, len;
+ int s;
+
+ DPRINTFN(5,("ucycomwritecb: status=%d\n", status));
+
+ if (status == USBD_CANCELLED || sc->sc_dying)
+ goto error;
+
+ if (status) {
+ DPRINTF(("ucycomwritecb: status=%d\n", status));
+ usbd_clear_endpoint_stall(sc->sc_hdev.sc_parent->sc_opipe);
+ /* XXX we should restart after some delay. */
+ goto error;
+ }
+
+ /* determine len the same way it was determined in ucycomstart */
+ /* rather than doing this, should we have just stored len in softc? */
+ cnt = ndqb(&tp->t_outq, 0);
+ len = cnt;
+ switch (sc->sc_olen) {
+ case 8:
+ if (cnt > sc->sc_olen - 1)
+ len = sc->sc_olen - 1;
+ break;
+ case 32:
+ if (cnt > sc->sc_olen - 2)
+ len = sc->sc_olen - 2;
+ break;
+ default:
+ goto error;
}
#ifdef UCYCOM_DEBUG
- ucycom_get_cfg(sc);
+ if (ucycomdebug) {
+ u_int32_t cc;
+
+ usbd_get_xfer_status(xfer, NULL, NULL, &cc, NULL);
+ DPRINTF(("ucycomwritecb: len = %d, cnt = %d, cc = %d\n", len, cnt, cc));
+
+ ucycom_get_cfg(sc);
+ }
#endif
- DPRINTFN(4,("ucycomstart: req %d chars did %d chars\n", cnt, len));
+
+ DPRINTFN(4,("ucycomwritecb: req %d chars did %d chars\n", cnt, len));
s = spltty();
CLR(tp->t_state, TS_BUSY);
@@ -584,8 +659,12 @@
else
ndflush(&tp->t_outq, len);
(*tp->t_linesw->l_start)(tp);
+ splx(s);
+ return;
-out:
+error:
+ s = spltty();
+ CLR(tp->t_state, TS_BUSY);
splx(s);
}
@@ -803,7 +882,7 @@
struct ucycom_softc *sc = ucycom_cd.cd_devs[UCYCOMUNIT(dev)];
struct tty *tp = sc->sc_tty;
int err;
-
+
DPRINTF(("ucycompoll: sc=%p, tp=%p, events=%d, lwp=%p\n", sc, tp,
events, l));
if (sc->sc_dying)
@@ -873,7 +952,7 @@
int (*rint)(int , struct tty *) = tp->t_linesw->l_rint;
uint8_t *cp = ibuf;
int s, n, st, chg;
-
+
/* We understand 8 byte and 32 byte input records */
switch (len) {
case 8:
@@ -986,13 +1065,13 @@
ucycom_dtr(struct ucycom_softc *sc, int set)
{
uint8_t old;
-
+
old = sc->sc_mcr;
if (set)
SET(sc->sc_mcr, UCYCOM_DTR);
else
CLR(sc->sc_mcr, UCYCOM_DTR);
-
+
if (old ^ sc->sc_mcr)
ucycom_set_status(sc);
}
@@ -1002,13 +1081,13 @@
ucycom_rts(struct ucycom_softc *sc, int set)
{
uint8_t old;
-
+
old = sc->sc_msr;
if (set)
SET(sc->sc_mcr, UCYCOM_RTS);
else
CLR(sc->sc_mcr, UCYCOM_RTS);
-
+
if (old ^ sc->sc_mcr)
ucycom_set_status(sc);
}
@@ -1024,7 +1103,7 @@
sc->sc_olen));
return;
}
-
+
DPRINTF(("ucycom_set_status: %d\n", sc->sc_mcr));
memset(sc->sc_obuf, 0, sc->sc_olen);
@@ -1042,7 +1121,7 @@
{
int err, cfg, baud;
uint8_t report[5];
-
+
err = uhidev_get_report(&sc->sc_hdev, UHID_FEATURE_REPORT,
report, sc->sc_flen);
cfg = report[4];
********************************************************