From: Mikolaj Kucharski Subject: Re: uncm(4) corrections To: Jan Schreiber Cc: tech@openbsd.org Date: Thu, 15 Oct 2026 16:07:15 +0000 Jan, I'm comparing initial diff from this thread to the one below. Question about changes in the uncm_txeof() function. Is the change between the diffs intentional? // diff v1 @@ -988,10 +1001,7 @@ uncm_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) { struct uncm_softc *sc = priv; struct ifnet *ifp = GET_IFP(sc); - int s; - s = splnet(); - ifq_clr_oactive(&ifp->if_snd); ifp->if_timer = 0; if (status != USBD_NORMAL_COMPLETION) { @@ -1003,10 +1013,9 @@ uncm_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) usbd_clear_endpoint_stall_async(sc->sc_tx_pipe); } } - if (ifq_empty(&ifp->if_snd) == 0) - uncm_start(ifp); - splx(s); + if (ifq_is_oactive(&ifp->if_snd)) + ifq_restart(&ifp->if_snd); } /* // diff v2 @@ -988,9 +1000,7 @@ uncm_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) { struct uncm_softc *sc = priv; struct ifnet *ifp = GET_IFP(sc); - int s; - s = splnet(); ifq_clr_oactive(&ifp->if_snd); ifp->if_timer = 0; @@ -1005,8 +1015,8 @@ uncm_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) } if (ifq_empty(&ifp->if_snd) == 0) uncm_start(ifp); - - splx(s); + if (ifq_is_oactive(&ifp->if_snd)) + ifq_restart(&ifp->if_snd); } /* Nit: You have a comment in the code which says: /* Drop packages that are too big for a NTB. */ It sounds better to me as: /* Drop packets that are too big for an NTB. */ On Wed, Oct 14, 2026 at 03:24:52PM +0000, Jan Schreiber wrote: > > > void > > > @@ -1215,8 +1220,8 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status) > > > break; > > > case UCDC_N_CONNECTION_SPEED_CHANGE: > > > if (total_len >= UCDC_NOTIFICATION_LENGTH + 8) { > > > - down = UGETDW(&sc->sc_intr_msg.data[0]); > > > - up = UGETDW(&sc->sc_intr_msg.data[4]); > > > + up = UGETDW(&sc->sc_intr_msg.data[0]); > > > + down = UGETDW(&sc->sc_intr_msg.data[4]); > > > > Are you sure this swap is correct? I'm looking at Table 21 on page > > 25 from CDC120_with_Errata_and_ECN_through_Apr_3_2025.pdf where I > > see: > > > > Offset 0 DLBitRate downlink bit rate, as sent on the IN pipe > > Offset 4 ULBitRate uplink bit rate, as sent on the OUT pipe > > Downloaded the updated spec and yes, you are correct (again) thank you! > I may have been confused due to the usb_cdc_connection_speed struct in > usbcdc.h. > > Corrected version below: > > diff --git sys/dev/usb/if_uncm.c sys/dev/usb/if_uncm.c > index 2cc6cf1cc4c..0297b296270 100644 > --- sys/dev/usb/if_uncm.c > +++ sys/dev/usb/if_uncm.c > @@ -403,11 +403,13 @@ uncm_detach(struct device *self, int flags) > if (ifp->if_flags & IFF_RUNNING) > uncm_stop(sc); > > - usb_rem_wait_task(sc->sc_udev, &sc->sc_link_task); > if (sc->sc_ctrl_pipe) { > usbd_close_pipe(sc->sc_ctrl_pipe); > sc->sc_ctrl_pipe = NULL; > } > + > + usb_rem_wait_task(sc->sc_udev, &sc->sc_link_task); > + > if (ifp->if_softc != NULL) { > ether_ifdetach(ifp); > if_detach(ifp); > @@ -480,10 +482,10 @@ uncm_ntb_setup(struct uncm_softc *sc) > sc->sc_ndp_div = UGETW(np.wNdpOutDivisor); > sc->sc_ndp_remainder = UGETW(np.wNdpOutPayloadRemainder); > > - if (!powerof2(sc->sc_align) || sc->sc_align == 0 || > + if (!powerof2(sc->sc_align) || sc->sc_align < 4 || > sc->sc_align >= sc->sc_tx_bufsz) > sc->sc_align = sizeof(uint32_t); > - if (!powerof2(sc->sc_ndp_div) || sc->sc_ndp_div == 0 || > + if (!powerof2(sc->sc_ndp_div) || sc->sc_ndp_div < 4 || > sc->sc_ndp_div >= sc->sc_tx_bufsz) > sc->sc_ndp_div = sizeof(uint32_t); > if (sc->sc_ndp_remainder >= sc->sc_ndp_div) > @@ -544,10 +546,10 @@ uncm_ntb_setup_format(struct uncm_softc *sc) > if (usbd_do_request(sc->sc_udev, &req, wFmt) != USBD_NORMAL_COMPLETION) > goto fail; > fmt = UGETW(wFmt); > - if ((sc->sc_ncm_supported_formats & (1UL << fmt)) == 0) > - goto fail; > if (fmt != NCM_FORMAT_NTB16 && fmt != NCM_FORMAT_NTB32) > goto fail; > + if ((sc->sc_ncm_supported_formats & (1UL << fmt)) == 0) > + goto fail; > sc->sc_ncm_format = fmt; > > DPRINTF("%s: using NCM format %d, supported 0x%x\n", > @@ -785,6 +787,14 @@ uncm_start(struct ifnet *ifp) > break; > > mlen = maxoverhead + m->m_pkthdr.len; > + if (ndgram == 0 && offs + mlen > sc->sc_tx_bufsz) { > + /* Drop packages that are too big for a NTB. */ > + ifq_deq_commit(&ifp->if_snd, m); > + m_freem(m); > + ifp->if_oerrors++; > + DPRINTF("%s: packet too large for NTB\n", DEVNAM(sc)); > + continue; > + } > if ((sc->sc_maxdgram != 0 && ndgram >= sc->sc_maxdgram) || > (offs + len + mlen > sc->sc_tx_bufsz)) { > ifq_deq_rollback(&ifp->if_snd, m); > @@ -803,9 +813,11 @@ uncm_start(struct ifnet *ifp) > } > if (ml_empty(&sc->sc_tx_ml)) > return; > - if (uncm_encap(sc, ndgram)) { > - ifq_set_oactive(&ifp->if_snd); > - ifp->if_timer = (2 * uncm_xfer_tout) / 1000; > + ifq_set_oactive(&ifp->if_snd); > + ifp->if_timer = (2 * uncm_xfer_tout) / 1000; > + if (!uncm_encap(sc, ndgram)) { > + ifp->if_timer = 0; > + ifq_clr_oactive(&ifp->if_snd); > } > } > > @@ -988,9 +1000,7 @@ uncm_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) > { > struct uncm_softc *sc = priv; > struct ifnet *ifp = GET_IFP(sc); > - int s; > > - s = splnet(); > ifq_clr_oactive(&ifp->if_snd); > ifp->if_timer = 0; > > @@ -1005,8 +1015,8 @@ uncm_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) > } > if (ifq_empty(&ifp->if_snd) == 0) > uncm_start(ifp); > - > - splx(s); > + if (ifq_is_oactive(&ifp->if_snd)) > + ifq_restart(&ifp->if_snd); > } > > /* > @@ -1017,7 +1027,6 @@ void > uncm_decap(struct uncm_softc *sc, struct usbd_xfer *xfer) > { > struct ifnet *ifp = GET_IFP(sc); > - int s; > void *buf; > uint32_t len; > char *dp; > @@ -1034,7 +1043,6 @@ uncm_decap(struct uncm_softc *sc, struct usbd_xfer *xfer) > > usbd_get_xfer_status(xfer, NULL, &buf, &len, NULL); > DPRINTFN(4, "%s: recv %d bytes\n", DEVNAM(sc), len); > - s = splnet(); > if (len < sizeof(*hdr16)) > goto toosmall; > > @@ -1160,13 +1168,11 @@ uncm_decap(struct uncm_softc *sc, struct usbd_xfer *xfer) > } > done: > if_input(ifp, &ml); > - splx(s); > return; > toosmall: > DPRINTF("%s: packet too small (%d)\n", DEVNAM(sc), len); > fail: > ifp->if_ierrors++; > - splx(s); > } > > void > @@ -1262,12 +1268,10 @@ uncm_link_task(void *arg) > { > struct uncm_softc *sc = arg; > struct ifnet *ifp = GET_IFP(sc); > - int s; > > if (usbd_is_dying(sc->sc_udev)) > return; > > - s = splnet(); > ifp->if_baudrate = sc->sc_baudrate; > if (sc->sc_link != LINK_STATE_UNKNOWN && > ifp->if_link_state != sc->sc_link) { > @@ -1281,7 +1285,6 @@ uncm_link_task(void *arg) > if_link_state_change(ifp); > if (LINK_STATE_IS_UP(sc->sc_link) && > (ifp->if_flags & IFF_RUNNING) && !ifq_empty(&ifp->if_snd)) > - uncm_start(ifp); > + ifq_start(&ifp->if_snd); > } > - splx(s); > } -- Regards, Mikolaj