| Message ID | 20180620110105.19955-28-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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
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;
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;
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(-)