From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: Fix dumping ISOCHRONOUS IN transfers
To: tech@openbsd.org
Date: Fri, 20 Dec 2024 20:57:06 +0100

On Fri, 20 Dec 2024 20:09:46 +0100,
Martin Pieuchot <mpi@grenadille.net> wrote:
> 
> I'd rather fix usb_tap() with a comment explaining why the hack I
> described above work and that it should be revisited if we start
> supporting a device like the one you found.
> 

Well, this approach make this diff quite clean:

Index: sys/dev/usb/usb.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/usb.c,v
diff -u -p -r1.133 usb.c
--- sys/dev/usb/usb.c	4 Sep 2024 07:54:52 -0000	1.133
+++ sys/dev/usb/usb.c	20 Dec 2024 19:41:58 -0000
@@ -952,7 +952,7 @@ usb_tap(struct usbd_bus *bus, struct usb
 		struct usbpcap_iso_hdr_full	uih;
 	} h;
 	struct usbpcap_pkt_hdr *uph = &h.uch.uch_hdr;
-	uint32_t nframes, offset;
+	uint32_t nframes, psize;
 	unsigned int bpfdir;
 	void *data = NULL;
 	size_t flen;
@@ -970,8 +970,11 @@ usb_tap(struct usbd_bus *bus, struct usb
 		uph->uph_xfertype = USBPCAP_TRANSFER_CONTROL;
 		break;
 	case UE_ISOCHRONOUS:
-		offset = 0;
 		nframes = xfer->nframes;
+		/* All our drivers use a fixed size (psize) for
+		 * ISOCHRONOUS packets. Calculate it to determine the
+		 * correct offset below. */
+		psize = xfer->length / nframes;
 #ifdef DIAGNOSTIC
 		if (nframes > _USBPCAP_MAX_ISOFRAMES) {
 			printf("%s: too many frames: %d > %d\n", __func__,
@@ -987,11 +990,12 @@ usb_tap(struct usbd_bus *bus, struct usb
 		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;
+			/* We can't use length, because IN frame may
+			 * have shorter length of packet whan expected. */
+			h.uih.uih_frames[i].uip_offset = i * psize;
 			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:
@@ -1049,6 +1053,12 @@ usb_tap(struct usbd_bus *bus, struct usb
 			if (xfer->rqflags & URQ_REQUEST)
 				h.uch.uch_stage = USBPCAP_CONTROL_STAGE_STATUS;
 		}
+	}
+
+	/* ISOCHRONOUS IN from device may have gaps, use full buffer */
+	if (bpfdir == BPF_DIRECTION_IN && uph->uph_dlen > 0 &&
+	    uph->uph_xfertype == USBPCAP_TRANSFER_ISOCHRONOUS) {
+		uph->uph_dlen = xfer->length;
 	}
 
 	/* Dump bulk/intr/iso data, ctrl DATA or STATUS stage. */



-- 
wbr, Kirill