27.11.2024 22:12, Kirill A. Korinsky пишет:
> 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.

I agree.

FWIW, video(1) works as before with this in a ThinkPad T14 gen3 Intel:

uvideo0 at uhub1 port 4 configuration 1 interface 0 "8SSC21D67422V1SR2C104ZJ Integrated RGB Camera" rev 2.01/10.21 addr 2
video0 at uvideo0
uvideo1 at uhub1 port 4 configuration 1 interface 2 "8SSC21D67422V1SR2C104ZJ Integrated RGB Camera" rev 2.01/10.21 addr 2
video1 at uvideo1


> 
> 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(
> 
>