From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [27/27] media: uvcvideo: use usb_fill_int_urb() From: Laurent Pinchart Message-Id: <3925059.Md1u3KRT1n@avalon> Date: Wed, 20 Jun 2018 14:55:23 +0300 To: Sebastian Andrzej Siewior Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab , linux-usb@vger.kernel.org, tglx@linutronix.de List-ID: SGkgU2ViYXN0aWFuLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBXZWRuZXNkYXksIDIw IEp1bmUgMjAxOCAxNDowMTowNSBFRVNUIFNlYmFzdGlhbiBBbmRyemVqIFNpZXdpb3Igd3JvdGU6 Cj4gVXNpbmcgdXNiX2ZpbGxfaW50X3VyYigpIGhlbHBzIHRvIGZpbmQgY29kZSB3aGljaCBpbml0 aWFsaXplcyBhbgo+IFVSQi4gQSBncmVwIGZvciBtZW1iZXJzIG9mIHRoZSBzdHJ1Y3QgKGxpa2Ug LT5jb21wbGV0ZSkgcmV2ZWFsIGxvdHMKPiBvZiBvdGhlciB0aGluZ3MsIHRvby4KPiB1c2JfZmls bF9pbnRfdXJiKCkgYWxzbyBjaGVja3MgYkludGVydmFsIHRvIGJlIGluIHRoZSAx4oCmMTYgcmFu Z2Ugb24KPiBIUy9TUy4KPiAKPiBDYzogTGF1cmVudCBQaW5jaGFydCA8bGF1cmVudC5waW5jaGFy dEBpZGVhc29uYm9hcmQuY29tPgo+IENjOiBNYXVybyBDYXJ2YWxobyBDaGVoYWIgPG1jaGVoYWJA a2VybmVsLm9yZz4KPiBTaWduZWQtb2ZmLWJ5OiBTZWJhc3RpYW4gQW5kcnplaiBTaWV3aW9yIDxi aWdlYXN5QGxpbnV0cm9uaXguZGU+Cj4gLS0tCj4gIGRyaXZlcnMvbWVkaWEvdXNiL3V2Yy91dmNf dmlkZW8uYyB8IDE0ICsrKysrKy0tLS0tLS0tCj4gIDEgZmlsZSBjaGFuZ2VkLCA2IGluc2VydGlv bnMoKyksIDggZGVsZXRpb25zKC0pCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbWVkaWEvdXNi L3V2Yy91dmNfdmlkZW8uYwo+IGIvZHJpdmVycy9tZWRpYS91c2IvdXZjL3V2Y192aWRlby5jIGlu ZGV4IGE4OGIyZTUxYTY2Ni4uNzllN2E4MjdlZDQ0IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvbWVk aWEvdXNiL3V2Yy91dmNfdmlkZW8uYwo+ICsrKyBiL2RyaXZlcnMvbWVkaWEvdXNiL3V2Yy91dmNf dmlkZW8uYwo+IEBAIC0xNjE5LDIxICsxNjE5LDE5IEBAIHN0YXRpYyBpbnQgdXZjX2luaXRfdmlk ZW9faXNvYyhzdHJ1Y3QgdXZjX3N0cmVhbWluZwo+ICpzdHJlYW0sIHJldHVybiAtRU5PTUVNOwo+ ICAJCX0KPiAKPiAtCQl1cmItPmRldiA9IHN0cmVhbS0+ZGV2LT51ZGV2Owo+IC0JCXVyYi0+Y29u dGV4dCA9IHN0cmVhbTsKPiAtCQl1cmItPnBpcGUgPSB1c2JfcmN2aXNvY3BpcGUoc3RyZWFtLT5k ZXYtPnVkZXYsCj4gLQkJCQllcC0+ZGVzYy5iRW5kcG9pbnRBZGRyZXNzKTsKPiArCQl1c2JfZmls bF9pbnRfdXJiKHVyYiwgc3RyZWFtLT5kZXYtPnVkZXYsCj4gKwkJCQkgdXNiX3Jjdmlzb2NwaXBl KHN0cmVhbS0+ZGV2LT51ZGV2LAo+ICsJCQkJCQkgZXAtPmRlc2MuYkVuZHBvaW50QWRkcmVzcyks Cj4gKwkJCQkgc3RyZWFtLT51cmJfYnVmZmVyW2ldLCBzaXplLAo+ICsJCQkJIHV2Y192aWRlb19j b21wbGV0ZSwgc3RyZWFtLAo+ICsJCQkJIGVwLT5kZXNjLmJJbnRlcnZhbCk7CgpZb3UncmUgZmls bGluZyBhbiBpc29jIFVSQiB3aXRoIHVzYl9maWxsX2ludF91cmIoKSwgd2hpY2ggaXMgZXhwbGlj aXRseSAKZG9jdW1lbnRlZCBhcyB1c2FibGUgdG8gZmlsbCBhbiBpbnRlcnJ1cHQgVVJCLiBTaG91 bGRuJ3Qgd2UgY3JlYXRlIGEgCnVzYl9maWxsX2lzb2NfdXJiKCkgZnVuY3Rpb24gPyBJdCBjb3Vs ZCBqdXN0IGJlIGFuIGFsaWFzIGZvciAKdXNiX2ZpbGxfaW50X3VyYigpIGlmIGlzb2MgYW5kIGlu dGVycnVwdCBVUkJzIGRvbid0IG5lZWQgdG8gYmUgdHJlYXRlZCAKZGlmZmVyZW50bHkuIEFsdGVy bmF0aXZlbHksIEknZCBiZSBmaW5lIHVzaW5nIHVzYl9maWxsX2ludF91cmIoKSBpZiB0aGUgCmZ1 bmN0aW9uIGRvY3VtZW50YXRpb24ncyB3YXMgdXBkYXRlZCB0byBtZW50aW9uIGlzb2MgVVJCcyBh cyB3ZWxsLgoKPiAgI2lmbmRlZiBDT05GSUdfRE1BX05PTkNPSEVSRU5UCj4gIAkJdXJiLT50cmFu c2Zlcl9mbGFncyA9IFVSQl9JU09fQVNBUCB8IFVSQl9OT19UUkFOU0ZFUl9ETUFfTUFQOwo+ICAJ CXVyYi0+dHJhbnNmZXJfZG1hID0gc3RyZWFtLT51cmJfZG1hW2ldOwo+ICAjZWxzZQo+ICAJCXVy Yi0+dHJhbnNmZXJfZmxhZ3MgPSBVUkJfSVNPX0FTQVA7Cj4gICNlbmRpZgo+IC0JCXVyYi0+aW50 ZXJ2YWwgPSBlcC0+ZGVzYy5iSW50ZXJ2YWw7CgpVbmxlc3MgSSdtIG1pc3Rha2VuIHRoaXMgaW50 cm9kdWNlcyBhIGNoYW5nZSBpbiBiZWhhdmlvdXIgZm9yIEhTIGFuZCBTUywgYW5kIApzaG91bGQg dGh1cyBiZSBkb2N1bWVudGVkIGluIHRoZSBjb21taXQgbWVzc2FnZS4gSSBzdXNwZWN0IHRoYXQg dGhpcyBpcyB0aGUgCnJlYWwgcmVhc29uIGZvciB0aGlzIHBhdGNoLiBJJ2QgdGh1cyB1cGRhdGUg dGhlIHN1YmplY3QgbGluZSB0byBkZXNjcmliZSB0aGlzIApmaXgsIGFuZCB0aGUgYm9keSBvZiB0 aGUgbWVzc2FnZSB0byBleHBsYWluIHdoeSB1c2luZyB1c2JfZmlsbF9pbnRfdXJiKCkgaXMgCnRo ZSBwcm9wZXIgZml4LgoKPiAtCQl1cmItPnRyYW5zZmVyX2J1ZmZlciA9IHN0cmVhbS0+dXJiX2J1 ZmZlcltpXTsKPiAtCQl1cmItPmNvbXBsZXRlID0gdXZjX3ZpZGVvX2NvbXBsZXRlOwo+ICAJCXVy Yi0+bnVtYmVyX29mX3BhY2tldHMgPSBucGFja2V0czsKPiAtCQl1cmItPnRyYW5zZmVyX2J1ZmZl cl9sZW5ndGggPSBzaXplOwoKdXNiX2ZpbGxfaW50X3VyYigpIHNldHMgdXJiLT5zdGFydF9mcmFt ZSB0byAtMS4gRG9lcyB0aGF0IGltcGFjdCB1cyBpbiBhbnkgd2F5IAo/Cgo+ICAJCWZvciAoaiA9 IDA7IGogPCBucGFja2V0czsgKytqKSB7Cj4gIAkJCXVyYi0+aXNvX2ZyYW1lX2Rlc2Nbal0ub2Zm c2V0ID0gaiAqIHBzaXplOwo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([213.167.242.64]:44556 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752884AbeFTLzI (ORCPT ); Wed, 20 Jun 2018 07:55:08 -0400 From: Laurent Pinchart To: Sebastian Andrzej Siewior Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab , linux-usb@vger.kernel.org, tglx@linutronix.de Subject: Re: [PATCH 27/27] media: uvcvideo: use usb_fill_int_urb() Date: Wed, 20 Jun 2018 14:55:23 +0300 Message-ID: <3925059.Md1u3KRT1n@avalon> In-Reply-To: <20180620110105.19955-28-bigeasy@linutronix.de> References: <20180620110105.19955-1-bigeasy@linutronix.de> <20180620110105.19955-28-bigeasy@linutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Sender: linux-media-owner@vger.kernel.org List-ID: 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=E2=80=A616 range = on > HS/SS. >=20 > Cc: Laurent Pinchart > Cc: Mauro Carvalho Chehab > Signed-off-by: Sebastian Andrzej Siewior > --- > drivers/media/usb/uvc/uvc_video.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) >=20 > diff --git a/drivers/media/usb/uvc/uvc_video.c > b/drivers/media/usb/uvc/uvc_video.c index a88b2e51a666..79e7a827ed44 1006= 44 > --- 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_streami= ng > *stream, return -ENOMEM; > } >=20 > - urb->dev =3D stream->dev->udev; > - urb->context =3D stream; > - urb->pipe =3D 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=20 documented as usable to fill an interrupt URB. Shouldn't we create a=20 usb_fill_isoc_urb() function ? It could just be an alias for=20 usb_fill_int_urb() if isoc and interrupt URBs don't need to be treated=20 differently. Alternatively, I'd be fine using usb_fill_int_urb() if the=20 function documentation's was updated to mention isoc URBs as well. > #ifndef CONFIG_DMA_NONCOHERENT > urb->transfer_flags =3D URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP; > urb->transfer_dma =3D stream->urb_dma[i]; > #else > urb->transfer_flags =3D URB_ISO_ASAP; > #endif > - urb->interval =3D ep->desc.bInterval; Unless I'm mistaken this introduces a change in behaviour for HS and SS, an= d=20 should thus be documented in the commit message. I suspect that this is the= =20 real reason for this patch. I'd thus update the subject line to describe th= is=20 fix, and the body of the message to explain why using usb_fill_int_urb() is= =20 the proper fix. > - urb->transfer_buffer =3D stream->urb_buffer[i]; > - urb->complete =3D uvc_video_complete; > urb->number_of_packets =3D npackets; > - urb->transfer_buffer_length =3D size; usb_fill_int_urb() sets urb->start_frame to -1. Does that impact us in any = way=20 ? > for (j =3D 0; j < npackets; ++j) { > urb->iso_frame_desc[j].offset =3D j * psize; =2D-=20 Regards, Laurent Pinchart