Re: CVS commit: src/sys/dev/usb

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Module Name:    src
> Committed By:   joe
> Date:           Thu Oct 22 17:17:47 UTC 2026
> 
> Modified Files:
>         src/sys/dev/usb: ohci.c
> 
> Log Message:
> use PR_ZERO on on pool allcoated memory and get rid of memset.
> 
> currently, xfer is zeroed using the wrong size.
> remove it and simplify code.

I think this change may not be correct.  I don't think pool_cache_get
respects PR_ZERO.  Perhaps we should add an assertion about this and
audit the system for it (or make it work by judiciously adding a
memset if PR_ZERO is passed).

I also suspect there was not a bug here before.  The code was
confusing because of the declared types, but correct:

--- ohci.c	31 Mar 2025 14:46:42 -0000	1.330
+++ ohci.c	22 Oct 2026 17:17:47 -0000	1.331
@@ -1112,14 +1112,12 @@ ohci_allocx(struct usbd_bus *bus, unsign
 	ohci_softc_t *sc = OHCI_BUS2SC(bus);
 	struct usbd_xfer *xfer;
 
-	xfer = pool_cache_get(sc->sc_xferpool, PR_WAITOK);
-	if (xfer != NULL) {
-		memset(xfer, 0, sizeof(struct ohci_xfer));
-
+	xfer = pool_cache_get(sc->sc_xferpool, PR_WAITOK | PR_ZERO);
 #ifdef DIAGNOSTIC
+	if (xfer != NULL) {
 		xfer->ux_state = XFER_BUSY;
-#endif
 	}
+#endif

Note that the declared type of xfer is struct usbd_xfer *, so with
this context it looks like memset(xfer, 0, sizeof(struct ohci_xfer))
is wrong and maybe should have been memset(xfer, 0, sizeof(*xfer)).

But the object allocated from sc->sc_xferpool actually sized for
struct ohci_xfer, so the original memset was fine:

	sc->sc_xferpool = pool_cache_init(sizeof(struct ohci_xfer), 0, 0, 0,
	    "ohcixfer", NULL, IPL_USB, NULL, NULL, NULL);

So I think this change did not fix a bug -- it actually _introduced_ a
bug by not zeroing the object, and should be reverted.

Perhaps we should rephrase this code to make it clearer (note that
pool_cache_get(sc->sc_xferpool PR_WAITOK) never fails, so there is no
need for a null test):

	struct ohci_xfer *ox;
	struct usbd_xfer *xfer;

	ox = pool_cache_get(sc->sc_xferpool, PR_WAITOK);
	memset(ox, 0, sizeof(*ox));
	xfer = &ox->xfer;
#ifdef DIAGNOSTIC
	xfer->ux_state = XFER_BUSY;
#endif
	return xfer;