Hi Bhupesh,
On Thursday 09 August 2012 01:19:49 Bhupesh SHARMA wrote:
> On Wednesday, August 08, 2012 5:32 PM Laurent Pinchart wrote:
> > On Tuesday 07 August 2012 23:07:56 Bhupesh SHARMA wrote:
> > > On Tuesday, August 07, 2012 8:23 PM Laurent Pinchart wrote:
> > > > On Tuesday 31 July 2012 06:24:51 Bhupesh Sharma wrote:
[snip]
> > > > > @@ -340,8 +344,13 @@ uvc_video_enable(struct uvc_video *video, int
> > > > > enable)
> > > > > }
> > > > >
> > > > > if (!enable) {
> > > > > - for (i = 0; i < UVC_NUM_REQUESTS; ++i)
> > > > > - usb_ep_dequeue(video->ep, video->req[i]);
> > > > > + /*
> > > > > + * dequeue requests on underlying UDC only if
> > > > > + * -ESHUTDOWN was not asserted by UDC earlier
> > > > > + */
> > > > > + if (!(queue->flags & UVC_QUEUE_DISCONNECTED))
> > > > > + for (i = 0; i < UVC_NUM_REQUESTS; ++i)
> > > > > + usb_ep_dequeue(video->ep, video->req[i]);
> > > >
> > > > What happens if you omit this check ?
> > >
> > > Actually the underlying UDCs clean-up their respective request list when
> > > they see a disconnect event from the Host.
> > >
> > > This makes them giveback all USB requests queued by an gadget with a
> > > status of ESHUTDOWN, so if we try to dequeue the 'already' dequeued
> > > requests from the UDC, it throws warning/errors that it doesn't have
> > > these requests queued any longer with it.
> >
> > OK. In that case, I'd like to fix the problem globally. When we run out of
> > V4L2 buffers the requests will end up being stored in the req_free list by
> > uvc_video_complete(). Those requests are not queued, and should thus not
> > be dequeued.
> >
> > Could you please split this change to another patch, and only dequeue
> > requests that are not queued ? This would require proper locking.
>
> Ok, but I have noticed that the underlying UDC will usually return all USB
> requests queued by the gadget when DISCONNECT happens.
>
> This means that no USB requests will be queued at UDC end when
> uvc_video_enable(0) is called. So if UDC saw a DISCONNECT from the Host,
> all the USB requests defined by the macro UVC_NUM_REQUESTS will become
> members of req_free list.
>
> So, in reality we don't have to dequeue any USB request on the UDC end, when
> uvc_video_enable(0) is called during DISCONNECT case (this is different from
> the case where user-space application itself working at the UVC gadget side
> is closed, in which case these requests will still be either queued to UDC
> or will be members of req_free list).
>
> In DISCONNECT from host, USB requests out of the UVC_NUM_REQUESTS count that
> were not already queued to the UDC are anyways not required to be dequeued
> and the rest have already been dequeued by the UDC itself. So, no USB
> requests need to be dequeued in this case.
I agree with you about the disconnection case. My point was that the UVC
gadget driver tries to dequeue all requests unconditionally (or, with your
patch, in the non-disconnection case only). In the normal, non-disconnection
case, some requests can be in a non-queued state when uvc_video_enable(0) is
called, so we would run into the same problem as in the disconnection case
(trying to dequeue a non-queued request). I'd like both issues to be fixed by
a single patch, split from this one.
> This is exactly what my patch does.
>
> Or am I missing something here?
>
> > Another option would be to turn the warning/error messages into debug
> > messages (some drivers - namely MUSB - already outputs a debug message in
> > the usb_ep_dequeue handler if the request is not queued). I wonder whether
> > that wouldn't be a better solution.
>
> Yes, but that isn't a very neat solution. We are trying to dequeue USB
> requests from UDC when there are none queued there (not sure many UDC
> maintainers will agree to this). As the gadget saw the USB requests
> completed with ESHUTDOWN status, it has the sufficient information to make a
> correct decision on which requests to dequeue now.
>
> But, please feel free to let me know your ideas on the same :)
I guess it depends on the usb_ep_dequeue() API. The function documentation
doesn't clearly state whether calling it with an already dequeuing request is
valid (in which case it should return an error, possibly print a debug
message, but not a warning/error message) or not (in which case it can be very
vocal about it, and even WARN_ON()).
--
Regards,
Laurent Pinchart
--
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
