From: Mikolaj Kucharski Subject: Re: uncm(4) cleanup To: Jan Schreiber Cc: tech@openbsd.org Date: Sat, 10 Oct 2026 14:41:31 +0000 Jan. With the diff below, which is on top of your diff, I have working Internet via uncm(4) device on Pixel 7a. --- if_uncm.c Sat Oct 10 13:52:08 2026 +++ if_uncm.c Sat Oct 10 13:35:32 2026 @@ -82,6 +82,7 @@ struct uncm_softc { struct usbd_interface *sc_ctrl_iface; struct usbd_pipe *sc_ctrl_pipe; struct usb_cdc_notification sc_intr_msg; + int sc_intr_size; struct usbd_interface *sc_data_iface; int sc_data_ifaceidx; @@ -190,7 +191,7 @@ uncm_attach(struct device *parent, struct device *self int current_ifaceno = -1; int data_ifaceno = -1; int mac_idx = -1; - int ctrl_ep; + int ctrl_ep, ctrl_maxpkt = 0; int altnum, i, s; uint32_t maxpktlen; @@ -282,8 +283,10 @@ uncm_attach(struct device *parent, struct device *self if (ed == NULL) break; if (UE_GET_XFERTYPE(ed->bmAttributes) == UE_INTERRUPT && - UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN) + UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN) { ctrl_ep = ed->bEndpointAddress; + ctrl_maxpkt = UE_GET_SIZE(UGETW(ed->wMaxPacketSize)); + } } if (ctrl_ep == -1) { @@ -338,8 +341,18 @@ uncm_attach(struct device *parent, struct device *self UE_GET_ADDR(ctrl_ep), data_ifaceno, UE_GET_ADDR(sc->sc_rx_ep), UE_GET_ADDR(sc->sc_tx_ep)); + /* + * Read one packet per transfer. Otherwise a full-sized + * SPEED_CHANGE (16 bytes) and the following NETWORK_CONNECTION + * get merged into one transfer and the latter is lost. + */ + sc->sc_intr_size = sizeof(sc->sc_intr_msg); + if (ctrl_maxpkt >= UCDC_NOTIFICATION_LENGTH && + ctrl_maxpkt < sc->sc_intr_size) + sc->sc_intr_size = ctrl_maxpkt; + if (usbd_open_pipe_intr(sc->sc_ctrl_iface, ctrl_ep, USBD_SHORT_XFER_OK, - &sc->sc_ctrl_pipe, sc, &sc->sc_intr_msg, sizeof(sc->sc_intr_msg), + &sc->sc_ctrl_pipe, sc, &sc->sc_intr_msg, sc->sc_intr_size, uncm_intr, USBD_DEFAULT_INTERVAL)) { printf("%s: failed to open control pipe\n", DEVNAM(sc)); goto fail; On Sat, Oct 10, 2026 at 01:00:24PM +0000, Mikolaj Kucharski wrote: > 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