Hi Laurent,
> Hi Bhupesh,
>
> On Tuesday 12 February 2013 16:48:08 Bhupesh SHARMA wrote:
> > > On Monday 11 February 2013 21:27:50 Bhupesh SHARMA wrote:
> > > > On Friday, February 08, 2013 4:22 AM Laurent Pinchart wrote:
> > > > > On Thursday 17 January 2013 16:23:50 Bhupesh Sharma wrote:
>
> [snip]
>
> > > > > > diff --git a/drivers/usb/gadget/uvc_video.c
> > > > > > b/drivers/usb/gadget/uvc_video.c index b0e53a8..f7d1913 100644
> > > > > > --- a/drivers/usb/gadget/uvc_video.c
> > > > > > +++ b/drivers/usb/gadget/uvc_video.c
> > > > > > @@ -235,7 +235,10 @@ uvc_video_alloc_requests(struct uvc_video
> > > > > > *video)
> > > > > > BUG_ON(video->req_size);
> > > > > >
> > > > > > for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
> > > > > >
> > > > > > - video->req_buffer[i] = kmalloc(video->ep-
> >maxpacket,
> > > > > > GFP_KERNEL);
> > > > > > + video->req_buffer[i] = kmalloc(video->ep-
> >maxpacket *
> > > > > > + video->ep->maxburst
> *
> > > > >
> > > > > If I'm not mistaken maxburst is 0 for FS and HS endpoints (from
> > > > > config_ep_by_speed in drivers/usb/gadget/composite.c). The
> > > > > buffer size will then be pretty small :-)
> > > >
> > > > Actually my initial patch had this approach:
> > > > video->req_buffer[i] = kmalloc(video->ep->maxpacket *
> > > > (video->ep->maxburst + 1) *
> > > > (video->ep->mult + 1))
> > > >
> > > > but due to a patch from Felipe:
> > > >
> > > > usb: gadget: composite: fix ep->maxburst initialization
> > > >
> > > > bMaxBurst field on endpoint companion descriptor is supposed to
> > > > contain the number of burst minus 1. When passing that to
> > > > controller drivers, we should be passing the real number instead
> > > > (by incrementing 1).
> > > >
> > > > While doing that, also fix the assumption on
> > > >
> > > > dwc3 that value comes decremented by one.
> > > >
> > > > Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> > > >
> > > > as we have now:
> > > > _ep->maxburst = comp_desc->bMaxBurst + 1;
> > > >
> > > > So, even if comp_desc->bMaxBurst is 0, _ep->maxburst will be 1.
> > > > So, this patch seems correct to me.
> > >
> > > For SS, sure, but if you connect the gadget to a USB 2.0 host
> > > maxburst will be set to 0, so buffer allocation will break.
> > >
> > > Please make sure you test all your patches with both USB 2.0 and USB
> > > 3.0 hosts.
> >
> > Ok. So will something like this suffice:
> > video->req_buffer[i] = kmalloc(video->ep->maxpacket *
> > (video->ep->comp_desc ?
> > video->ep->maxburst : 1) *
> > (video->ep->mult + 1),
> > GFP_KERNEL);
> >
> > As ep->comp_desc will be NULL for HS and FS devices..
>
> What about the following patch ?
>
> diff --git a/drivers/usb/gadget/uvc_video.c
> b/drivers/usb/gadget/uvc_video.c index cd067a6..f5193cc 100644
> --- a/drivers/usb/gadget/uvc_video.c
> +++ b/drivers/usb/gadget/uvc_video.c
> @@ -229,13 +229,18 @@ uvc_video_free_requests(struct uvc_video *video)
> static int uvc_video_alloc_requests(struct uvc_video *video) {
> + unsigned int req_size;
> unsigned int i;
> int ret = -ENOMEM;
>
> BUG_ON(video->req_size);
>
> + req_size = video->ep->maxpacket
> + * max_t(unsigned int, video->ep->maxburst, 1)
> + * (video->ep->mult + 1);
> +
> for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
> - video->req_buffer[i] = kmalloc(video->ep->maxpacket,
> GFP_KERNEL);
> + video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);
> if (video->req_buffer[i] == NULL)
> goto error;
>
Yes, this also seems fine. But I think the companion descriptor can be easily associated as been absent for HS/FS devices.
But even this will do. Better to add a one line comment on top of this calculation though for better readibility.
I will do the same for V4 then..
Regards,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
