Download raw body.
uncm(4) corrections
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
uncm(4) corrections