[27/27] media: uvcvideo: use usb_fill_int_urb()

[27/27] media: uvcvideo: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

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@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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);
 #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;
-		urb->transfer_buffer = stream->urb_buffer[i];
-		urb->complete = uvc_video_complete;
 		urb->number_of_packets = npackets;
-		urb->transfer_buffer_length = size;
 
 		for (j = 0; j < npackets; ++j) {
 			urb->iso_frame_desc[j].offset = j * psize;

[27/27] media: uvcvideo: use usb_fill_int_urb()

[Laurent Pinchart]

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@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  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;

[27/27] media: uvcvideo: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 14:55:23 [+0300], Laurent Pinchart wrote:
> Hi Sebastian,
Hi Laurent,

> 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@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  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.

I thought I read it there but I couldn't find it. And then I found it in
Documentation/driver-api/usb/URB.rst:
| you specify. You can use the :c:func:`usb_fill_int_urb` macro to fill
| most ISO transfer fields.

So you simply asking that the kerneldoc of usb_fill_int_urb() is
extended to mention isoc, too?

> >  #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 did:

| usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
| HS/SS.

so this wasn't enough?

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

Actually no. I was looking for all the ->complete handlers and most of
them used usb_fill_… except a few. And while moving to the function I
was checking if everything stays the same (and mentioned ->interval
since ->start_frame is documented as a return parameter).

So here you are asking for a description update which explicit says
bug-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 
> ?

It should not. The documentation says:
|  * @start_frame: Returns the initial frame for isochronous transfers.

> >  		for (j = 0; j < npackets; ++j) {
> >  			urb->iso_frame_desc[j].offset = j * psize;

Sebastian
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[27/27] media: uvcvideo: use usb_fill_int_urb()

[Laurent Pinchart]

Hi Sebastian,

On Wednesday, 20 June 2018 16:21:44 EEST Sebastian Andrzej Siewior wrote:
> On 2018-06-20 14:55:23 [+0300], Laurent Pinchart wrote:
> > Hi Sebastian,
> 
> Hi Laurent,
> 
> > 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@ideasonboard.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > ---
> > > 
> > >  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.
> 
> I thought I read it there but I couldn't find it. And then I found it in
> 
> Documentation/driver-api/usb/URB.rst:
> | you specify. You can use the :c:func:`usb_fill_int_urb` macro to fill
> | most ISO transfer fields.
> 
> So you simply asking that the kerneldoc of usb_fill_int_urb() is
> extended to mention isoc, too?

That would be nice I think.

> > >  #ifndef CONFIG_DMA_NONCOHERENT
> > >  		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> > >  		urb->transfer_dma = stream->urb_dma[i];
> > >  #else
> > >  
> > >  #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 did:
> | usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
> | HS/SS.
> 
> so this wasn't enough?

This sounds to me like a sanity check, while the patch effectively changes the 
code as follows for HS and SS:

-		urb->interval = ep->desc.bInterval;
+		urb->interval = 1 << (ep->desc.bInterval - 1);

I believe the change is correct, although it would be nice if you could 
double-check, as the documentation of the function states:

 * @interval: what to set the urb interval to, encoded like
 *      the endpoint descriptor's bInterval value.

> > 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.
> 
> Actually no. I was looking for all the ->complete handlers and most of
> them used usb_fill_… except a few. And while moving to the function I
> was checking if everything stays the same (and mentioned ->interval
> since ->start_frame is documented as a return parameter).
> 
> So here you are asking for a description update which explicit says
> bug-fix?

I'd like that, as it seems to be a bugfix, not just a code cleanup without any 
behavioural change.

> > > -		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 ?
> 
> It should not. The documentation says:
> |  * @start_frame: Returns the initial frame for isochronous transfers.

Thanks for checking.

> > >  		for (j = 0; j < npackets; ++j) {
> > >  		
> > >  			urb->iso_frame_desc[j].offset = j * psize;