Hi Sebastian,
Thank you for the patch.
On Wednesday, 20 June 2018 14:01:05 EEST Sebastian Andrzej Siewior wrote:
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.
> usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
> HS/SS.
>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index a88b2e51a666..79e7a827ed44 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming
> *stream, return -ENOMEM;
> }
>
> - urb->dev = stream->dev->udev;
> - urb->context = stream;
> - urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> - ep->desc.bEndpointAddress);
> + usb_fill_int_urb(urb, stream->dev->udev,
> + usb_rcvisocpipe(stream->dev->udev,
> + ep->desc.bEndpointAddress),
> + stream->urb_buffer[i], size,
> + uvc_video_complete, stream,
> + ep->desc.bInterval);
You're filling an isoc URB with usb_fill_int_urb(), which is explicitly
documented as usable to fill an interrupt URB. Shouldn't we create a
usb_fill_isoc_urb() function ? It could just be an alias for
usb_fill_int_urb() if isoc and interrupt URBs don't need to be treated
differently. Alternatively, I'd be fine using usb_fill_int_urb() if the
function documentation's was updated to mention isoc URBs as well.
> #ifndef CONFIG_DMA_NONCOHERENT
> urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> urb->transfer_dma = stream->urb_dma[i];
> #else
> urb->transfer_flags = URB_ISO_ASAP;
> #endif
> - urb->interval = ep->desc.bInterval;
Unless I'm mistaken this introduces a change in behaviour for HS and SS, and
should thus be documented in the commit message. I suspect that this is the
real reason for this patch. I'd thus update the subject line to describe this
fix, and the body of the message to explain why using usb_fill_int_urb() is
the proper fix.
> - urb->transfer_buffer = stream->urb_buffer[i];
> - urb->complete = uvc_video_complete;
> urb->number_of_packets = npackets;
> - urb->transfer_buffer_length = size;
usb_fill_int_urb() sets urb->start_frame to -1. Does that impact us in any way
?
> for (j = 0; j < npackets; ++j) {
> urb->iso_frame_desc[j].offset = j * psize;
--
Regards,
Laurent Pinchart
