Download raw body.
uncm(4) cleanup
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<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,AUTOCONF6TEMP,AUTOCONF6,AUTOCONF4> 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
uncm(4) cleanup