On Wed, 27 Nov 2024 19:09:47 +0100,
Stuart Henderson <stu@spacehopper.org> wrote:
> 
> On 2024/11/27 17:54, Kirill A. Korinsky wrote:
> >  
> > +	/* do device negotiation with commit after set alternate interface */
> 
> would "renegotiate after setting alternate interface" make sense?
>

Thanks, I think renegotiate is better.

> > +	error = uvideo_vs_negotiation(sc, 1);
> > +	if (error != USBD_NORMAL_COMPLETION)
> > +		printf("%s: could not negotiate after setting alternate interface!\n",
> > +		    DEVNAME(sc));
> > +
> >  	DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x\n",
> >  	    DEVNAME(sc), sc->sc_vs_cur->endpoint);
> 
> please stick to <80 columns (either shorter text, or split the
> string across two lines)
>
> should that return (error) too?
> 

This is a point that I'm thinking a lot. From one hand it makes perfect
sense to return error here, from another hand it may break some webcam which
doesn't support it, but may continue to work, or may not.

Thus, Linux uses 0 as a hardcoded alternate interface, has comments that
soem device crashes without it, and returns error:
https://github.com/torvalds/linux/blob/v6.12/drivers/media/usb/uvc/uvc_video.c#L2180-L2220

Base on all this return error is right move, I guess.

Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.223 uvideo.c
--- sys/dev/usb/uvideo.c	27 Nov 2024 11:37:23 -0000	1.223
+++ sys/dev/usb/uvideo.c	27 Nov 2024 18:45:42 -0000
@@ -1899,6 +1899,14 @@ uvideo_vs_open(struct uvideo_softc *sc)
 		return (USBD_INVAL);
 	}
 
+	/* renegotiate with commit after setting alternate interface */
+	error = uvideo_vs_negotiation(sc, 1);
+	if (error != USBD_NORMAL_COMPLETION) {
+		printf("%s: could not renegotiate after setting "
+		    "alternate interface!\n", DEVNAME(sc));
+		return (error);
+	}
+
 	DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x\n",
 	    DEVNAME(sc), sc->sc_vs_cur->endpoint);
 	error = usbd_open_pipe(


-- 
wbr, Kirill