From: Mikolaj Kucharski Subject: Re: uncm(4) corrections To: Jan Schreiber Cc: tech@openbsd.org Date: Wed, 14 Oct 2026 14:33:42 +0000 The diff didn't apply. $ patch -p0 -N -C < uncm-corrections.patch ... Patching file sys/dev/usb/if_uncm.c using Plan A... Hunk #1 succeeded at 403. Hunk #2 succeeded at 482. Hunk #3 succeeded at 546. Hunk #4 succeeded at 787. Hunk #5 succeeded at 814. Hunk #6 succeeded at 1001. Hunk #7 succeeded at 1013. Hunk #8 succeeded at 1026. Hunk #9 succeeded at 1042. Hunk #10 succeeded at 1167. Hunk #11 failed at 1220. Hunk #12 succeeded at 1267 (offset 27 lines). Hunk #13 succeeded at 1284 (offset 27 lines). 1 out of 13 hunks failed done See my comment below about DLBitRate / ULBitRate. On Wed, Oct 14, 2026 at 01:02:15PM +0000, Jan Schreiber wrote: > This patch does the following things: > > - removes unneeded splnet() calls > - removes packages too big for a single NTB from the queue so they > won't block > - corrects the order for checking NCM_FORMATs. now the format > check happens before the usage of the device supplied value > - use the ifqueue(9) api to enqueue packages > - up and down transfer speeds where mixed around (no change in > behaviour but the debug messages will be correct now) > - be more spec compliant for with checking the wNdpOutAlignment to be > at least 4 and not == 0 > - close the pipe and then call usb_rem_wait_task(9) > > Tested locally with my Android device. > > Jan > > > diff --git sys/dev/usb/if_uncm.c sys/dev/usb/if_uncm.c > index 4c34080208f..5360c7bf938 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,15 @@ 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 +814,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,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); > } > > /* > @@ -1017,7 +1026,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 +1042,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 +1167,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 > @@ -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 > sc->sc_baudrate = MIN(up, down); > DPRINTFN(2, "%s: speed down %u up %u\n", DEVNAM(sc), > down, up); > @@ -1235,12 +1240,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) { > @@ -1254,7 +1257,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