On Tue, Feb 20, 2018 at 03:05:33PM +0100, Martin Pieuchot wrote:
> Diff below implements the USBPcap interface for dumping USB isochronous
> frames. It can be very useful to analyze what the stack is doing. Note
> that tcpdump(8)'s snaplen default is too small to capture such frames. I
> used the following to analyze uaudio(4) traffic on ehci(4):
>
> # tcpdump -s 3303 -w usnd.pcap -i usb0
>
> The first capture I did was with AUDIODEVICE=snd/0 then I did a second
> one with AUDIODEVICE=rsnd/0. Like Stefan I hear some crackling noise
> in the first case.
>
> What I could see after opening the pcap(3) files in wireshark confirmed
> my theory. When using the raw audio device delay between frames is less
> than 8us. However in the general case it jumps to 35us.
>
> It is too early to say what needs to be fixed, but at least I'd like to
> get the USBPcap part in, ok?
Response from the peanut gallery...
This seems like a cool thing to have. I can try to test this later
when I get home from work.
>
> Index: usb.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/usb.c,v
> retrieving revision 1.117
> diff -u -p -r1.117 usb.c
> --- usb.c 19 Feb 2018 09:20:45 -0000 1.117
> +++ usb.c 20 Feb 2018 13:34:43 -0000
> @@ -992,11 +992,17 @@ usb_tap(struct usbd_bus *bus, struct usb
> #if NBPFILTER > 0
> struct usb_softc *sc = (struct usb_softc *)bus->usbctl;
> usb_endpoint_descriptor_t *ed = xfer->pipe->endpoint->edesc;
> - struct usbpcap_ctl_hdr uch;
> - struct usbpcap_pkt_hdr *uph = &uch.uch_hdr;
> + union {
> + struct usbpcap_ctl_hdr uch;
> + struct usbpcap_iso_hdr_full uih;
> + } h;
> + struct usbpcap_pkt_hdr *uph = &h.uch.uch_hdr;
> + uint32_t nframes, offset;
> unsigned int bpfdir;
> void *data = NULL;
> + size_t flen;
> caddr_t bpf;
> + int i;
>
> bpf = bus->bpf;
> if (bpf == NULL)
> @@ -1004,13 +1010,34 @@ usb_tap(struct usbd_bus *bus, struct usb
>
> switch (UE_GET_XFERTYPE(ed->bmAttributes)) {
> case UE_CONTROL:
> - /* Control transfer headers include an extra byte. */
> - uph->uph_hlen = htole16(sizeof(uch));
> + /* Control transfer headers include an extra byte */
> + uph->uph_hlen = htole16(sizeof(struct usbpcap_ctl_hdr));
> uph->uph_xfertype = USBPCAP_TRANSFER_CONTROL;
> break;
> case UE_ISOCHRONOUS:
> - uph->uph_hlen = htole16(sizeof(*uph));
> + offset = 0;
> + nframes = xfer->nframes;
> +#ifdef DIAGNOSTIC
> + if (nframes > _USBPCAP_MAX_ISOFRAMES) {
> + printf("%s: too many frames: %d > %d\n", __func__,
> + xfer->nframes, _USBPCAP_MAX_ISOFRAMES);
> + nframes = _USBPCAP_MAX_ISOFRAMES;
> + }
> +#endif
> + /* Isochronous transfer headers include space for one frame */
> + flen = (nframes - 1) * sizeof(struct usbpcap_iso_pkt);
> + uph->uph_hlen = htole16(sizeof(struct usbpcap_iso_hdr) + flen);
> uph->uph_xfertype = USBPCAP_TRANSFER_ISOCHRONOUS;
> + h.uih.uih_startframe = 0; /* not yet used */
> + h.uih.uih_nframes = nframes;
> + h.uih.uih_errors = 0; /* we don't have per-frame error */
> + for (i = 0; i < nframes; i++) {
> + h.uih.uih_frames[i].uip_offset = offset;
> + h.uih.uih_frames[i].uip_length = xfer->frlengths[i];
> + /* See above, we don't have per-frame error */
> + h.uih.uih_frames[i].uip_status = 0;
> + offset += xfer->frlengths[i];
> + }
> break;
> case UE_BULK:
> uph->uph_hlen = htole16(sizeof(*uph));
> @@ -1034,7 +1061,7 @@ usb_tap(struct usbd_bus *bus, struct usb
>
> /* Outgoing control requests start with a STAGE dump. */
> if ((xfer->rqflags & URQ_REQUEST) && (dir == USBTAP_DIR_OUT)) {
> - uch.uch_stage = USBPCAP_CONTROL_STAGE_SETUP;
> + h.uch.uch_stage = USBPCAP_CONTROL_STAGE_SETUP;
> uph->uph_dlen = sizeof(usb_device_request_t);
> bpf_tap_hdr(bpf, uph, uph->uph_hlen, &xfer->request,
> uph->uph_dlen, BPF_DIRECTION_OUT);
> @@ -1045,11 +1072,13 @@ usb_tap(struct usbd_bus *bus, struct usb
> if (!usbd_xfer_isread(xfer)) {
> data = KERNADDR(&xfer->dmabuf, 0);
> uph->uph_dlen = xfer->length;
> - uch.uch_stage = USBPCAP_CONTROL_STAGE_DATA;
> + if (xfer->rqflags & URQ_REQUEST)
> + h.uch.uch_stage = USBPCAP_CONTROL_STAGE_DATA;
> } else {
> data = NULL;
> uph->uph_dlen = 0;
> - uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
> + if (xfer->rqflags & URQ_REQUEST)
> + h.uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
> }
> } else { /* USBTAP_DIR_IN */
> bpfdir = BPF_DIRECTION_IN;
> @@ -1057,11 +1086,13 @@ usb_tap(struct usbd_bus *bus, struct usb
> if (usbd_xfer_isread(xfer)) {
> data = KERNADDR(&xfer->dmabuf, 0);
> uph->uph_dlen = xfer->actlen;
> - uch.uch_stage = USBPCAP_CONTROL_STAGE_DATA;
> + if (xfer->rqflags & URQ_REQUEST)
> + h.uch.uch_stage = USBPCAP_CONTROL_STAGE_DATA;
> } else {
> data = NULL;
> uph->uph_dlen = 0;
> - uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
> + if (xfer->rqflags & URQ_REQUEST)
> + h.uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
> }
> }
>
> @@ -1070,8 +1101,8 @@ usb_tap(struct usbd_bus *bus, struct usb
>
> /* Incoming control requests with DATA need a STATUS stage. */
> if ((xfer->rqflags & URQ_REQUEST) && (dir == USBTAP_DIR_IN) &&
> - (uch.uch_stage == USBPCAP_CONTROL_STAGE_DATA)) {
> - uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
> + (h.uch.uch_stage == USBPCAP_CONTROL_STAGE_DATA)) {
> + h.uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
> uph->uph_dlen = 0;
> bpf_tap_hdr(bpf, uph, uph->uph_hlen, NULL, 0, BPF_DIRECTION_IN);
> }
> Index: usbpcap.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/usbpcap.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 usbpcap.h
> --- usbpcap.h 3 Feb 2018 13:37:37 -0000 1.1
> +++ usbpcap.h 20 Feb 2018 13:33:43 -0000
> @@ -53,4 +53,35 @@ struct usbpcap_ctl_hdr {
> #define USBPCAP_CONTROL_STAGE_STATUS 2
> } __attribute__((packed));
>
> +struct usbpcap_iso_pkt {
> + uint32_t uip_offset;
> + uint32_t uip_length;
> + uint32_t uip_status;
> +} __attribute__((packed));
> +
> +/*
> + * Header used when dumping isochronous transfers.
> + */
> +struct usbpcap_iso_hdr {
> + struct usbpcap_pkt_hdr uih_hdr;
> + uint32_t uih_startframe;
> + uint32_t uih_nframes; /* number of frame */
> + uint32_t uih_errors; /* error count */
> + struct usbpcap_iso_pkt uih_frames[1];
> +} __attribute__((packed));
> +
> +#ifdef _KERNEL
> +/*
> + * OpenBSD specific, maximum number of frames per transfer used across
> + * all USB drivers. This allows us to setup the header on the stack.
> + */
> +#define _USBPCAP_MAX_ISOFRAMES 40
> +struct usbpcap_iso_hdr_full {
> + struct usbpcap_pkt_hdr uih_hdr;
> + uint32_t uih_startframe;
> + uint32_t uih_nframes;
> + uint32_t uih_errors;
> + struct usbpcap_iso_pkt uih_frames[_USBPCAP_MAX_ISOFRAMES];
> +} __attribute__((packed));
> +#endif /* _KERNEL */
> #endif /* _USBCAP_H_ */
>
