From: Mikolaj Kucharski <mikolaj@kucharski.name>
Subject: Re: uncm(4): process all notifications in one interrupt transfer
To: Jan Schreiber <jes@posteo.de>
Cc: tech@openbsd.org, stsp@stsp.name
Date: Mon, 12 Oct 2026 19:49:15 +0000

On Mon, Oct 12, 2026 at 06:39:52PM +0000, Jan Schreiber wrote:
> On Mon, Oct 12, 2026 at 05:36:17PM +0000, Mikolaj Kucharski wrote:
> > Jan.
> >
> > Thank you looking into this. See my comments below.
> 
> You are correct, I only copied the header. Thank you!
> Added the missing memcpy to msg.data below.

This looks better. I cannot spot anything wrong, but this is far outside
of my expertise area.

One whitespace related comment below in the diff.

I didn't had time yet to run your diff on my machine. I am planning to
do so, however next days are busy for me.


> diff --git sys/dev/usb/if_uncm.c sys/dev/usb/if_uncm.c
> index a003841a7b7..3723459bb2f 100644
> --- sys/dev/usb/if_uncm.c
> +++ sys/dev/usb/if_uncm.c
> @@ -1174,7 +1174,7 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  {
>  	struct uncm_softc *sc = priv;
>  	struct ifnet *ifp = GET_IFP(sc);
> -	struct usb_cdc_notification *msg;
> +	struct usb_cdc_notification msg;
>  	uint32_t up, down;
>  	uint8_t *buf;
>  	int	total_len, off, dlen;
> @@ -1213,8 +1213,8 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  	buf = (uint8_t *)&sc->sc_intr_msg;
>  	for (off = 0; off + UCDC_NOTIFICATION_LENGTH <= total_len;
>  	    off += UCDC_NOTIFICATION_LENGTH + dlen) {
> -		msg = (struct usb_cdc_notification *)(buf + off);
> -		dlen = UGETW(msg->wLength);
> +		memcpy(&msg, buf + off, UCDC_NOTIFICATION_LENGTH);
> +		dlen = UGETW(msg.wLength);
> 
>  		if (off + UCDC_NOTIFICATION_LENGTH + dlen > total_len) {
>  			DPRINTF("%s: truncated notification at offset %d\n",
> @@ -1223,26 +1223,29 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  		}
> 
>  		/* Framing is lost; do not parse the rest. */
> -		if (msg->bmRequestType != UCDC_NOTIFICATION) {
> +		if (msg.bmRequestType != UCDC_NOTIFICATION) {
>  			DPRINTF("%s: unexpected notification (type=0x%02x)\n",
> -			    DEVNAM(sc), msg->bmRequestType);
> +			    DEVNAM(sc), msg.bmRequestType);
>  			break;
>  		}
> 
> -		switch (msg->bNotification) {
> +		memcpy(msg.data, buf + off + UCDC_NOTIFICATION_LENGTH,
> +	    	    MIN(dlen, sizeof(msg.data)));

There are some spaces between tabs somewhere in the above line.


> +
> +		switch (msg.bNotification) {
>  		case UCDC_N_NETWORK_CONNECTION:
>  			if (ifp->if_flags & IFF_DEBUG)
>  				log(LOG_DEBUG, "%s: network %sconnected\n",
>  				    DEVNAM(sc),
> -				    UGETW(msg->wValue) ? "" : "dis");
> -			sc->sc_link = UGETW(msg->wValue) ?
> +				    UGETW(msg.wValue) ? "" : "dis");
> +			sc->sc_link = UGETW(msg.wValue) ?
>  			    LINK_STATE_UP : LINK_STATE_DOWN;
>  			usb_add_task(sc->sc_udev, &sc->sc_link_task);
>  			break;
>  		case UCDC_N_CONNECTION_SPEED_CHANGE:
>  			if (dlen >= 8) {
> -				down = UGETDW(&msg->data[0]);
> -				up = UGETDW(&msg->data[4]);
> +				down = UGETDW(&msg.data[0]);
> +				up = UGETDW(&msg.data[4]);
>  				sc->sc_baudrate = MIN(up, down);
>  				DPRINTFN(2, "%s: speed down %u up %u\n",
>  				    DEVNAM(sc), down, up);
> @@ -1251,7 +1254,7 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  			break;
>  		default:
>  			DPRINTF("%s: unexpected notification (0x%02x)\n",
> -			    DEVNAM(sc), msg->bNotification);
> +			    DEVNAM(sc), msg.bNotification);
>  			break;
>  		}
>  	}

-- 
Regards,
 Mikolaj