On 09/24/2013 03:01 PM, Hans Verkuil wrote:
On Thu 12 September 2013 12:19:29 Sylwester Nawrocki wrote:
On 09/11/2013 04:01 PM, Hans Verkuil wrote:
On 09/11/2013 03:07 PM, Sylwester Nawrocki wrote:
On 09/09/2013 11:07 AM, Hans Verkuil wrote:
On 09/06/2013 12:33 AM, Sylwester Nawrocki wrote:
On 08/07/2013 07:49 PM, Hans Verkuil wrote:
On 08/07/2013 06:49 PM, Sylwester Nawrocki wrote:
On 08/02/2013 03:00 PM, Hans Verkuil wrote:
On 08/02/2013 02:27 PM, Sylwester Nawrocki wrote:
[...]
So there is lots of things that may fail after first video node is
registered, and on which open() may be called immediately.
The only solution I know off-hand to handle this safely is to unregister all
nodes if some fail, but to return 0 in the probe function. If an open() succeeded,
then that will just work until the node is closed, at which point the v4l2_device
release() is called and you can cleanup.
Another solution would be to properly implement struct video_device::release()
callback and in video device open() do video_is_registered() check while
holding the driver private video device lock. Also video_unregister_device()
would need to be called while holding the video device lock.
How would that help?
By not allowing video_unregister_device() call in the middle of the file op and
serializing whatever does video device unregistration with the file ops. I'm not
sure it is possible in all cases.
That does not actually help you. An open() call may succeed during probe(), but
if an error occurs in the probe() afterwards and the device is unregistered and
all internal data released, you still crash.
That's true, a mutex within the driver's private structure couldn't be held
while the data structure is being freed.
I guess I had in mind the code that I actually tested, where I added a
driver
private kref, which was decremented in remove() (and the failed
probe()). And
incremented upon video device registration. But that's just too
convoluted to be
considered a generic solution.
While probe() is in progress you want to block all open()s.
Since all video_device structs now have a v4l2_device pointer, what about
doing this in v4l2_open():
if (test_bit(V4L2_DEV_FL_IN_PROBE,&vdev->v4l2_dev->flags))
return -EAGAIN;
It's not ideal, but this allows drivers with a complex probe sequence
to set the flag and clear it at the end. While it is set, all opens will
return EAGAIN.
Sounds interesting, I guess that could solve the problem. I suppose user
space
would handle it gracefully. Since the device as a whole is not ready I guess
I would be OK to return such an error.
I initially thought the flag should be per video_device but it indeed seems
more reasonable to make it a global flag for all video nodes associated with
struct v4l2_dev.
I'll try to find couple minutes to actually tests this. :)
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
