From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: support pre UVC 1.5 devices
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Tue, 12 Aug 2025 22:48:32 +0200

On Tue, Aug 12, 2025 at 06:30:50PM +0200, Kirill A. Korinsky wrote:

> On Tue, 12 Aug 2025 13:22:46 +0200,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> > 
> > > @@ -2044,13 +2045,18 @@ uvideo_vs_get_probe(struct uvideo_softc 
> > >  
> > >  	pc = (struct usb_video_probe_commit *)probe_data;
> > >  
> > > -	error = usbd_do_request(sc->sc_udev, &req, probe_data);
> > > -	if (error) {
> > > +	error = usbd_do_request_flags(sc->sc_udev, &req, probe_data, 0,
> > > +	    &actlen, USBD_DEFAULT_TIMEOUT);
> > 
> > I'm OK if we accept short transfers here, but have you tried to use
> > USBD_SHORT_XFER_OK as an usbd_do_request_flags() flag?  Then it shouldn't
> > be required to make a specific error check on it.
> > 
> 
> I haven't know that. Thank makes this diff much cleaner.
> 
> > > +	if (error != USBD_NORMAL_COMPLETION && error != USBD_SHORT_XFER) {
> > >  		printf("%s: could not GET probe request: %s\n",
> > >  		    DEVNAME(sc), usbd_errstr(error));
> > >  		return (USBD_INVAL);
> > >  	}
> > >  
> > > +	/* Different UVC version defines different length of a probe */
> > > +	bzero(probe_data + actlen,
> > > +	    sizeof(struct usb_video_probe_commit ) - actlen);
> > > +
> > 
> > Why is it required to bzero probe_data here?  There is one use of
> > uvideo_vs_get_probe() currently, and that always bzero's probe_data
> > before:
> > 
> >         /* get probe */
> >         bzero(probe_data, sizeof(probe_data));
> >         error = uvideo_vs_get_probe(sc, probe_data, GET_CUR);
> > 
> > On a short transfer I wouldn't expect the rest of probe_data to contain
> > garbage, but I have no device here to test that.
> >
> 
> Yes, so do I.
> 
> From antoher hand clean it up, to avoid garbage from some devices seems not
> that bad idea.

Well, fine for me.  Just wanted to understand if there is a specific
reason for it.  But could you please tweak it a bit:

> > > +	/* Different UVC version defines different length of a probe */

A bit more clear comment would be nice, so we still understand this
when reading it in two years.  Something like:

/*
 * Different UVC version defines different length of a probe;
 * Make sure that the unused porition of probe_data remains zero.
 */

> > > +	bzero(probe_data + actlen,
> > > +	    sizeof(struct usb_video_probe_commit ) - actlen);

Can you please remove the space here?           ^