Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 17 Feb 2011 19:00:39 +0100
From:      Hans Petter Selasky <hselasky@c2i.net>
To:        freebsd-multimedia@freebsd.org
Subject:   Re: USB MIDI patch for FreeBSD 8/9
Message-ID:  <201102171900.39165.hselasky@c2i.net>
In-Reply-To: <201102171118.40721.hselasky@c2i.net>
References:  <201102171118.40721.hselasky@c2i.net>

next in thread | previous in thread | raw e-mail | index | archive | help
--Boundary-00=_HJWXN7NIxEvKAlB
Content-Type: Text/Plain;
  charset="iso-8859-15"
Content-Transfer-Encoding: 7bit

On Thursday 17 February 2011 11:18:40 Hans Petter Selasky wrote:
> Dear all USB MIDI freaks using FreeBSD 8+!
> 
> After some more testing I'm going to push a patch for the USB AUDIO driver
> that improves USB MIDI on FreeBSD. I'm sending the patch out for review so
> that users get a chance to try out the changes before they get committed.
> The patch should apply to 8.2-RELEASE aswell as 9-CURRENT.
> 
> How to apply:
> 
> cd /usr/src/sys/dev/sound/usb/
> cat umidi_patch_001.txt | patch
> 
> A short summary of what the changes are about:
> 
> - Make the USB MIDI driver more OSS compliant by implementing the
> correct data format for the /dev/umidiX.Y character devices. Also
> implement some missing OSS MIDI ioctls.
> - Use the USB stack's builtin clear-stall feature.
> - Wrap some long lines.
> - Use memcpy() instead of bcopy().
- Use memset() instead of bzero().
> 
> --HPS

Found some bugs in my initial patch after testing. Attaching new one.

--HPS

--Boundary-00=_HJWXN7NIxEvKAlB
Content-Type: text/plain; charset="iso-8859-15"; name="umidi_patch_002.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
	filename="umidi_patch_002.txt"

=== uaudio.c
==================================================================
--- uaudio.c	(revision 218769)
+++ uaudio.c	(local)
@@ -191,10 +191,15 @@
 	uint8_t	iface_alt_index;
 };
 
-#define	UMIDI_N_TRANSFER    4		/* units */
 #define	UMIDI_CABLES_MAX   16		/* units */
 #define	UMIDI_BULK_SIZE  1024		/* bytes */
 
+enum {
+	UMIDI_TX_TRANSFER,
+	UMIDI_RX_TRANSFER,
+	UMIDI_N_TRANSFER,
+};
+
 struct umidi_sub_chan {
 	struct usb_fifo_sc fifo;
 	uint8_t *temp_cmd;
@@ -224,10 +229,6 @@
 	uint8_t	iface_index;
 	uint8_t	iface_alt_index;
 
-	uint8_t	flags;
-#define	UMIDI_FLAG_READ_STALL  0x01
-#define	UMIDI_FLAG_WRITE_STALL 0x02
-
 	uint8_t	read_open_refcount;
 	uint8_t	write_open_refcount;
 
@@ -336,9 +337,7 @@
 static usb_callback_t uaudio_chan_play_callback;
 static usb_callback_t uaudio_chan_record_callback;
 static usb_callback_t uaudio_mixer_write_cfg_callback;
-static usb_callback_t umidi_read_clear_stall_callback;
 static usb_callback_t umidi_bulk_read_callback;
-static usb_callback_t umidi_write_clear_stall_callback;
 static usb_callback_t umidi_bulk_write_callback;
 
 static void	uaudio_chan_fill_info_sub(struct uaudio_softc *,
@@ -493,7 +492,7 @@
 
 static const struct usb_config
 	umidi_config[UMIDI_N_TRANSFER] = {
-	[0] = {
+	[UMIDI_TX_TRANSFER] = {
 		.type = UE_BULK,
 		.endpoint = UE_ADDR_ANY,
 		.direction = UE_DIR_OUT,
@@ -502,7 +501,7 @@
 		.callback = &umidi_bulk_write_callback,
 	},
 
-	[1] = {
+	[UMIDI_RX_TRANSFER] = {
 		.type = UE_BULK,
 		.endpoint = UE_ADDR_ANY,
 		.direction = UE_DIR_IN,
@@ -510,26 +509,6 @@
 		.flags = {.pipe_bof = 1,.short_xfer_ok = 1,.proxy_buffer = 1,},
 		.callback = &umidi_bulk_read_callback,
 	},
-
-	[2] = {
-		.type = UE_CONTROL,
-		.endpoint = 0x00,	/* Control pipe */
-		.direction = UE_DIR_ANY,
-		.bufsize = sizeof(struct usb_device_request),
-		.callback = &umidi_write_clear_stall_callback,
-		.timeout = 1000,	/* 1 second */
-		.interval = 50,	/* 50ms */
-	},
-
-	[3] = {
-		.type = UE_CONTROL,
-		.endpoint = 0x00,	/* Control pipe */
-		.direction = UE_DIR_ANY,
-		.bufsize = sizeof(struct usb_device_request),
-		.callback = &umidi_read_clear_stall_callback,
-		.timeout = 1000,	/* 1 second */
-		.interval = 50,	/* 50ms */
-	},
 };
 
 static devclass_t uaudio_devclass;
@@ -1577,10 +1556,10 @@
 uaudio_mixer_add_ctl_sub(struct uaudio_softc *sc, struct uaudio_mixer_node *mc)
 {
 	struct uaudio_mixer_node *p_mc_new =
-	malloc(sizeof(*p_mc_new), M_USBDEV, M_WAITOK);
+	    malloc(sizeof(*p_mc_new), M_USBDEV, M_WAITOK);
 
-	if (p_mc_new) {
-		bcopy(mc, p_mc_new, sizeof(*p_mc_new));
+	if (p_mc_new != NULL) {
+		memcpy(p_mc_new, mc, sizeof(*p_mc_new));
 		p_mc_new->next = sc->sc_mixer_root;
 		sc->sc_mixer_root = p_mc_new;
 		sc->sc_mixer_count++;
@@ -1722,7 +1701,7 @@
 
 	DPRINTFN(3, "ichs=%d ochs=%d\n", ichs, ochs);
 
-	bzero(&mix, sizeof(mix));
+	memset(&mix, 0, sizeof(mix));
 
 	mix.wIndex = MAKE_WORD(d0->bUnitId, sc->sc_mixer_iface_no);
 	uaudio_mixer_determine_class(&iot[id], &mix);
@@ -1782,7 +1761,7 @@
 	if (d->bNrInPins == 0) {
 		return;
 	}
-	bzero(&mix, sizeof(mix));
+	memset(&mix, 0, sizeof(mix));
 
 	mix.wIndex = MAKE_WORD(d->bUnitId, sc->sc_mixer_iface_no);
 	mix.wValue[0] = MAKE_WORD(0, 0);
@@ -1852,7 +1831,7 @@
 	if (d->bControlSize == 0) {
 		return;
 	}
-	bzero(&mix, sizeof(mix));
+	memset(&mix, 0, sizeof(mix));
 
 	nchan = (d->bLength - 7) / d->bControlSize;
 	mmask = uaudio_mixer_feature_get_bmaControls(d, 0);
@@ -1986,7 +1965,7 @@
 		DPRINTF("no mode select\n");
 		return;
 	}
-	bzero(&mix, sizeof(mix));
+	memset(&mix, 0, sizeof(mix));
 
 	mix.wIndex = MAKE_WORD(d0->bUnitId, sc->sc_mixer_iface_no);
 	mix.nchan = 1;
@@ -2012,7 +1991,7 @@
 	struct uaudio_mixer_node mix;
 	uint16_t ptype;
 
-	bzero(&mix, sizeof(mix));
+	memset(&mix, 0, sizeof(mix));
 
 	ptype = UGETW(d0->wProcessType);
 
@@ -2067,7 +2046,7 @@
 	}
 	if (d1->bmControls[0] & UA_EXT_ENABLE_MASK) {
 
-		bzero(&mix, sizeof(mix));
+		memset(&mix, 0, sizeof(mix));
 
 		mix.wIndex = MAKE_WORD(d0->bUnitId, sc->sc_mixer_iface_no);
 		mix.nchan = 1;
@@ -2294,7 +2273,7 @@
 	}
 error:
 	DPRINTF("bad data\n");
-	bzero(&r, sizeof(r));
+	memset(&r, 0, sizeof(r));
 done:
 	return (r);
 }
@@ -3284,25 +3263,12 @@
  *========================================================================*/
 
 static void
-umidi_read_clear_stall_callback(struct usb_xfer *xfer, usb_error_t error)
-{
-	struct umidi_chan *chan = usbd_xfer_softc(xfer);
-	struct usb_xfer *xfer_other = chan->xfer[1];
-
-	if (usbd_clear_stall_callback(xfer, xfer_other)) {
-		DPRINTF("stall cleared\n");
-		chan->flags &= ~UMIDI_FLAG_READ_STALL;
-		usbd_transfer_start(xfer_other);
-	}
-}
-
-static void
 umidi_bulk_read_callback(struct usb_xfer *xfer, usb_error_t error)
 {
 	struct umidi_chan *chan = usbd_xfer_softc(xfer);
 	struct umidi_sub_chan *sub;
 	struct usb_page_cache *pc;
-	uint8_t buf[1];
+	uint8_t buf[5];
 	uint8_t cmd_len;
 	uint8_t cn;
 	uint16_t pos;
@@ -3320,60 +3286,66 @@
 
 		while (actlen >= 4) {
 
-			usbd_copy_out(pc, pos, buf, 1);
-
-			cmd_len = umidi_cmd_to_len[buf[0] & 0xF];	/* command length */
-			cn = buf[0] >> 4;	/* cable number */
+			/* copy out the MIDI data */
+			usbd_copy_out(pc, pos, buf, 4);
+			/* command length */
+			cmd_len = umidi_cmd_to_len[buf[0] & 0xF];
+			/* cable number */
+			cn = buf[0] >> 4;
+			/*
+			 * Lookup sub-channel. The index is range
+			 * checked below.
+			 */
 			sub = &chan->sub[cn];
 
-			if (cmd_len && (cn < chan->max_cable) && sub->read_open) {
-				usb_fifo_put_data(sub->fifo.fp[USB_FIFO_RX], pc,
-				    pos + 1, cmd_len, 1);
-			} else {
-				/* ignore the command */
-			}
+			if ((cmd_len != 0) &&
+			    (cn < chan->max_cable) &&
+			    (sub->read_open != 0)) {
 
+				/* re-pack into a 4-byte array */
+				/* zero unused bytes */
+				if (cmd_len == 1) {
+					buf[2] = 0;
+					buf[3] = 0;
+					buf[4] = 0;
+				} else if (cmd_len == 2) {
+					buf[3] = 0;
+					buf[4] = 0;
+				} else {
+					buf[4] = 0;
+				}
+
+				/*
+				 * Send data in 4-byte chunks to the
+				 * application:
+				 */
+				usb_fifo_put_data_linear(
+				    sub->fifo.fp[USB_FIFO_RX],
+				    buf + 1, 4, 1);
+			}
 			actlen -= 4;
 			pos += 4;
 		}
 
 	case USB_ST_SETUP:
 		DPRINTF("start\n");
-
-		if (chan->flags & UMIDI_FLAG_READ_STALL) {
-			usbd_transfer_start(chan->xfer[3]);
-			return;
-		}
+tr_setup:
 		usbd_xfer_set_frame_len(xfer, 0, usbd_xfer_max_len(xfer));
 		usbd_transfer_submit(xfer);
-		return;
+		break;
 
 	default:
 		DPRINTF("error=%s\n", usbd_errstr(error));
 
 		if (error != USB_ERR_CANCELLED) {
 			/* try to clear stall first */
-			chan->flags |= UMIDI_FLAG_READ_STALL;
-			usbd_transfer_start(chan->xfer[3]);
+			usbd_xfer_set_stall(xfer);
+			goto tr_setup;
 		}
-		return;
-
+		break;
 	}
 }
 
-static void
-umidi_write_clear_stall_callback(struct usb_xfer *xfer, usb_error_t error)
-{
-	struct umidi_chan *chan = usbd_xfer_softc(xfer);
-	struct usb_xfer *xfer_other = chan->xfer[0];
-
-	if (usbd_clear_stall_callback(xfer, xfer_other)) {
-		DPRINTF("stall cleared\n");
-		chan->flags &= ~UMIDI_FLAG_WRITE_STALL;
-		usbd_transfer_start(xfer_other);
-	}
-}
-
 /*
  * The following statemachine, that converts MIDI commands to
  * USB MIDI packets, derives from Linux's usbmidi.c, which
@@ -3502,6 +3474,8 @@
 			sub->temp_cmd = sub->temp_1;
 			sub->state = UMIDI_ST_SYSEX_0;
 			return (1);
+		default:
+			break;
 		}
 	}
 	return (0);
@@ -3527,13 +3501,9 @@
 		DPRINTF("actlen=%d bytes\n", len);
 
 	case USB_ST_SETUP:
-
+tr_setup:
 		DPRINTF("start\n");
 
-		if (chan->flags & UMIDI_FLAG_WRITE_STALL) {
-			usbd_transfer_start(chan->xfer[2]);
-			return;
-		}
 		total_length = 0;	/* reset */
 		start_cable = chan->curr_cable;
 		tr_any = 0;
@@ -3593,7 +3563,7 @@
 			usbd_xfer_set_frame_len(xfer, 0, total_length);
 			usbd_transfer_submit(xfer);
 		}
-		return;
+		break;
 
 	default:			/* Error */
 
@@ -3601,11 +3571,10 @@
 
 		if (error != USB_ERR_CANCELLED) {
 			/* try to clear stall first */
-			chan->flags |= UMIDI_FLAG_WRITE_STALL;
-			usbd_transfer_start(chan->xfer[2]);
+			usbd_xfer_set_stall(xfer);
+			goto tr_setup;
 		}
-		return;
-
+		break;
 	}
 }
 
@@ -3635,7 +3604,7 @@
 {
 	struct umidi_chan *chan = usb_fifo_softc(fifo);
 
-	usbd_transfer_start(chan->xfer[1]);
+	usbd_transfer_start(chan->xfer[UMIDI_RX_TRANSFER]);
 }
 
 static void
@@ -3662,7 +3631,7 @@
 {
 	struct umidi_chan *chan = usb_fifo_softc(fifo);
 
-	usbd_transfer_start(chan->xfer[0]);
+	usbd_transfer_start(chan->xfer[UMIDI_TX_TRANSFER]);
 }
 
 static void
@@ -3677,8 +3646,7 @@
 
 	if (--(chan->write_open_refcount) == 0) {
 		DPRINTF("(stopping write transfer)\n");
-		usbd_transfer_stop(chan->xfer[2]);
-		usbd_transfer_stop(chan->xfer[0]);
+		usbd_transfer_stop(chan->xfer[UMIDI_TX_TRANSFER]);
 	}
 }
 
@@ -3703,7 +3671,7 @@
 		}
 		/* clear stall first */
 		mtx_lock(&chan->mtx);
-		chan->flags |= UMIDI_FLAG_WRITE_STALL;
+		usbd_xfer_set_stall(chan->xfer[UMIDI_TX_TRANSFER]);
 		chan->write_open_refcount++;
 		sub->write_open = 1;
 
@@ -3769,7 +3737,8 @@
 		DPRINTF("setting of alternate index failed!\n");
 		goto detach;
 	}
-	usbd_set_parent_iface(sc->sc_udev, chan->iface_index, sc->sc_mixer_iface_index);
+	usbd_set_parent_iface(sc->sc_udev, chan->iface_index,
+	    sc->sc_mixer_iface_index);
 
 	error = usbd_transfer_setup(uaa->device, &chan->iface_index,
 	    chan->xfer, umidi_config, UMIDI_N_TRANSFER,
@@ -3799,13 +3768,15 @@
 	mtx_lock(&chan->mtx);
 
 	/* clear stall first */
-	chan->flags |= UMIDI_FLAG_READ_STALL;
+	usbd_xfer_set_stall(chan->xfer[UMIDI_RX_TRANSFER]);
 
 	/*
-	 * NOTE: at least one device will not work properly unless
-	 * the BULK pipe is open all the time.
+	 * NOTE: At least one device will not work properly unless the
+	 * BULK IN pipe is open all the time. This might have to do
+	 * about that the internal queues of the device overflow if we
+	 * don't read them regularly.
 	 */
-	usbd_transfer_start(chan->xfer[1]);
+	usbd_transfer_start(chan->xfer[UMIDI_RX_TRANSFER]);
 
 	mtx_unlock(&chan->mtx);
 
@@ -3828,8 +3799,7 @@
 
 	mtx_lock(&chan->mtx);
 
-	usbd_transfer_stop(chan->xfer[3]);
-	usbd_transfer_stop(chan->xfer[1]);
+	usbd_transfer_stop(chan->xfer[UMIDI_RX_TRANSFER]);
 
 	mtx_unlock(&chan->mtx);
 

--Boundary-00=_HJWXN7NIxEvKAlB--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201102171900.39165.hselasky>