From: Mikolaj Kucharski Subject: Re: uncm(4) cleanup To: Jan Schreiber Cc: tech@openbsd.org Date: Sat, 10 Oct 2026 13:00:24 +0000 Hi Jan. I ran this diff on -current, however with and without below diff, I see no carrier on uncm0 interface. $ dmesg | grep -e ^uncm uncm0 at uhub1 port 1 configuration 1 interface 0 "Google Pixel 7a" rev 2.10/5.10 addr 6 uncm0: address 26:d3:84:63:d2:cb $ ifconfig uncm0 uncm0: flags=a48843 mtu 1500 lladdr 26:d3:84:63:d2:cb index 3 priority 0 llprio 3 status: no carrier inet6 fe80::24d3:84ff:fe63:d2cb%uncm0 prefixlen 64 scopeid 0x3 // with the below diff $ sysctl -n kern.version OpenBSD 8.0-current (GENERIC.MP) #11: Sat Oct 10 12:22:21 UTC 2026 root@pc1.home.local:/home/mkucharski/openbsd/src/sys/arch/amd64/compile/GENERIC.MP // or vanilla kernel $ sysctl -n kern.version OpenBSD 8.0-current (GENERIC.MP) #86: Sat Oct 10 02:01:31 MDT 2026 deraadt@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP On the Android side USB tethering is turned on. On Fri, Oct 09, 2026 at 01:46:40PM +0000, Jan Schreiber wrote: > After dlg@ send me a link to his inital implementation of a cdc ncm > driver I took some time fixing up some whitespace, added a macro for > more readability and added some missing status checks. > > Jan > > diff --git sys/dev/usb/if_uncm.c sys/dev/usb/if_uncm.c > index 626fb552dff..1579235bd07 100644 > --- sys/dev/usb/if_uncm.c > +++ sys/dev/usb/if_uncm.c > @@ -108,6 +108,13 @@ struct uncm_softc { > > #define UNCM_RX_NTB_MAX 16384 > > +#define UNCM_NTB16_MINSZ (sizeof(struct ncm_header16) + \ > + sizeof(struct ncm_pointer16) + \ > + sizeof(struct ncm_pointer16_dgram)) > +#define UNCM_NTB32_MINSZ (sizeof(struct ncm_header32) + \ > + sizeof(struct ncm_pointer32) + \ > + sizeof(struct ncm_pointer32_dgram)) > + > int uncm_match(struct device *, void *, void *); > void uncm_attach(struct device *, struct device *, void *); > int uncm_detach(struct device *, int); > @@ -192,16 +199,17 @@ uncm_attach(struct device *parent, struct device *self, void *aux) > sc->sc_ctrl_ifaceno = uaa->ifaceno; > sc->sc_maxpktlen = ETHER_MAX_LEN - ETHER_CRC_LEN; > sc->sc_link = LINK_STATE_UNKNOWN; > + > ml_init(&sc->sc_tx_ml); > usb_init_task(&sc->sc_link_task, uncm_link_task, sc, > USB_TASK_TYPE_GENERIC); > + > usbd_desc_iter_init(sc->sc_udev, &iter); > while ((desc = usbd_desc_iter_next(&iter))) { > if (desc->bDescriptorType == UDESC_IFACE_ASSOC) { > ad = (usb_interface_assoc_descriptor_t *)desc; > if (ad->bFirstInterface == uaa->ifaceno && > ad->bInterfaceCount > 1) { > - /* Fall back to the interface following control. */ > data_ifaceno = uaa->ifaceno + 1; > } > continue; > @@ -245,6 +253,7 @@ uncm_attach(struct device *parent, struct device *self, void *aux) > break; > } > } > + > if (data_ifaceno == -1) { > printf("%s: no data interface number\n", DEVNAM(sc)); > goto fail; > @@ -260,6 +269,7 @@ uncm_attach(struct device *parent, struct device *self, void *aux) > usbd_claim_iface(sc->sc_udev, i); > } > } > + > if (sc->sc_data_iface == NULL) { > printf("%s: no data interface found\n", DEVNAM(sc)); > goto fail; > @@ -275,6 +285,7 @@ uncm_attach(struct device *parent, struct device *self, void *aux) > UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN) > ctrl_ep = ed->bEndpointAddress; > } > + > if (ctrl_ep == -1) { > printf("%s: missing interrupt endpoint\n", DEVNAM(sc)); > goto fail; > @@ -316,6 +327,7 @@ uncm_attach(struct device *parent, struct device *self, void *aux) > UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_OUT) > sc->sc_tx_ep = ed->bEndpointAddress; > } > + > if (sc->sc_rx_ep == -1 || sc->sc_tx_ep == -1) { > printf("%s: missing bulk endpoints\n", DEVNAM(sc)); > goto fail; > @@ -438,12 +450,14 @@ uncm_ntb_setup(struct uncm_softc *sc) > { > usb_device_request_t req; > struct ncm_ntb_parameters np; > + int toosmall = 0; > > req.bmRequestType = UT_READ_CLASS_INTERFACE; > req.bRequest = NCM_GET_NTB_PARAMETERS; > USETW(req.wValue, 0); > USETW(req.wIndex, sc->sc_ctrl_ifaceno); > USETW(req.wLength, sizeof(np)); > + > if (usbd_do_request(sc->sc_udev, &req, &np) == USBD_NORMAL_COMPLETION && > UGETW(np.wLength) == sizeof(np)) { > sc->sc_rx_bufsz = MIN(UGETDW(np.dwNtbInMaxSize), UINT16_MAX); > @@ -476,19 +490,17 @@ uncm_ntb_setup(struct uncm_softc *sc) > sc->sc_ncm_supported_formats = NCM_FORMAT_NTB16_MASK; > } > > - if (sc->sc_rx_bufsz < sizeof(struct ncm_header16) + > - sizeof(struct ncm_pointer16) + > - sizeof(struct ncm_pointer16_dgram) || > - sc->sc_tx_bufsz < sizeof(struct ncm_header16) + > - sizeof(struct ncm_pointer16) + > - sizeof(struct ncm_pointer16_dgram) || > - ((sc->sc_ncm_supported_formats & NCM_FORMAT_NTB32_MASK) && > - (sc->sc_rx_bufsz < sizeof(struct ncm_header32) + > - sizeof(struct ncm_pointer32) + > - sizeof(struct ncm_pointer32_dgram) || > - sc->sc_tx_bufsz < sizeof(struct ncm_header32) + > - sizeof(struct ncm_pointer32) + > - sizeof(struct ncm_pointer32_dgram)))) { > + if (sc->sc_rx_bufsz < UNCM_NTB16_MINSZ || > + sc->sc_tx_bufsz < UNCM_NTB16_MINSZ) > + toosmall = 1; > + > + if (sc->sc_ncm_supported_formats & NCM_FORMAT_NTB32_MASK) { > + if (sc->sc_rx_bufsz < UNCM_NTB32_MINSZ || > + sc->sc_tx_bufsz < UNCM_NTB32_MINSZ) > + toosmall = 1; > + } > + > + if (toosmall) { > DPRINTF("%s: invalid NTB size %d/%d\n", DEVNAM(sc), > sc->sc_rx_bufsz, sc->sc_tx_bufsz); > sc->sc_ncm_supported_formats = 0; > @@ -549,6 +561,7 @@ uncm_ntb_setup_input_size(struct uncm_softc *sc) > USETW(req.wValue, 0); > USETW(req.wIndex, sc->sc_ctrl_ifaceno); > USETW(req.wLength, sizeof(dwSize)); > + > if (usbd_do_request(sc->sc_udev, &req, dwSize) == > USBD_NORMAL_COMPLETION) > sc->sc_rx_bufsz = UNCM_RX_NTB_MAX; > @@ -566,11 +579,11 @@ uncm_alloc_xfers(struct uncm_softc *sc) > sc->sc_rx_bufsz); > } > > - if (sc->sc_tx_xfer == NULL) { > + if (sc->sc_tx_xfer == NULL) { > if ((sc->sc_tx_xfer = usbd_alloc_xfer(sc->sc_udev)) != NULL) > sc->sc_tx_buf = usbd_alloc_buffer(sc->sc_tx_xfer, > sc->sc_tx_bufsz); > - } > + } > > return (sc->sc_rx_buf != NULL && sc->sc_tx_buf != NULL); > } > @@ -584,13 +597,13 @@ uncm_free_xfers(struct uncm_softc *sc) > sc->sc_rx_buf = NULL; > } > > - if (sc->sc_tx_xfer != NULL) { > + if (sc->sc_tx_xfer != NULL) { > usbd_free_xfer(sc->sc_tx_xfer); > sc->sc_tx_xfer = NULL; > sc->sc_tx_buf = NULL; > } > > - ml_purge(&sc->sc_tx_ml); > + ml_purge(&sc->sc_tx_ml); > } > > void > @@ -713,6 +726,7 @@ uncm_padding(void *buf, size_t bufsz, int offs, int alignment, int remainder) > nb = uncm_align(bufsz, offs, alignment, remainder); > if (nb > 0) > memset(buf + offs, 0, nb); > + > return nb; > } > > @@ -757,7 +771,7 @@ uncm_start(struct ifnet *ifp) > if (m == NULL) > break; > > - mlen = maxoverhead + m->m_pkthdr.len; > + mlen = maxoverhead + m->m_pkthdr.len; > if ((sc->sc_maxdgram != 0 && ndgram >= sc->sc_maxdgram) || > (offs + len + mlen > sc->sc_tx_bufsz)) { > ifq_deq_rollback(&ifp->if_snd, m); > @@ -852,7 +866,7 @@ uncm_encap(struct uncm_softc *sc, int ndgram) > int offs = 0, plen = 0; > int dgoffs = 0, poffs; > struct mbuf *m; > - usbd_status err; > + usbd_status err; > > KASSERT(sc->sc_ncm_format == NCM_FORMAT_NTB16 || > sc->sc_ncm_format == NCM_FORMAT_NTB32); > @@ -1051,9 +1065,11 @@ uncm_decap(struct uncm_softc *sc, struct usbd_xfer *xfer) > > if ((uint64_t)len < ptroff + sizeof(*ptr16)) > goto toosmall; > + > ptr16 = (struct ncm_pointer16 *)(buf + ptroff); > psig = UGETDW(ptr16->dwSignature); > ptrlen = UGETW(ptr16->wLength); > + > if ((uint64_t)len < (uint64_t)ptrlen + ptroff) > goto toosmall; > > @@ -1152,18 +1168,23 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status) > return; > > if (status != USBD_NORMAL_COMPLETION) { > + if (status == USBD_NOT_STARTED || status == USBD_CANCELLED) > + return; > DPRINTF("%s: notification error: %s\n", DEVNAM(sc), > usbd_errstr(status)); > if (status == USBD_STALLED) > usbd_clear_endpoint_stall_async(sc->sc_ctrl_pipe); > return; > } > + > usbd_get_xfer_status(xfer, NULL, NULL, &total_len, NULL); > + > if (total_len < UCDC_NOTIFICATION_LENGTH) { > DPRINTF("%s: short notification (%d<%d)\n", DEVNAM(sc), > total_len, UCDC_NOTIFICATION_LENGTH); > return; > } > + > if (sc->sc_intr_msg.bmRequestType != UCDC_NOTIFICATION) { > DPRINTF("%s: unexpected notification (type=0x%02x)\n", > DEVNAM(sc), sc->sc_intr_msg.bmRequestType); > @@ -1213,7 +1234,8 @@ uncm_link_task(void *arg) > if (ifp->if_flags & IFF_DEBUG) > log(LOG_DEBUG, "%s: link state changed from %s to %s\n", > DEVNAM(sc), > - LINK_STATE_IS_UP(ifp->if_link_state) ? "up" : "down", > + LINK_STATE_IS_UP(ifp->if_link_state) ? > + "up" : "down", > LINK_STATE_IS_UP(sc->sc_link) ? "up" : "down"); > ifp->if_link_state = sc->sc_link; > if_link_state_change(ifp); > @@ -1223,4 +1245,3 @@ uncm_link_task(void *arg) > } > splx(s); > } > - > > -- Regards, Mikolaj