Hi Hans,
On Fri, Jul 24, 2015 at 11:08:06AM +0200, Hans Verkuil wrote:
> Hi Sakari,
>
> On 07/22/2015 06:14 PM, Sakari Ailus wrote:
> > By default, serialise open and release internal ops using a mutex.
> >
> > The underlying problem is that a large proportion of the drivers do use
> > v4l2_fh_is_singular() in their open() handlers (struct
> > v4l2_subdev_internal_ops). v4l2_subdev_open(), .open file operation handler
> > of the V4L2 sub-device framework, calls v4l2_fh_add() which adds the file
> > handle to the list of open file handles of the device. Later on in the
> > open() handler of the sub-device driver uses v4l2_fh_is_singular() to
> > determine whether it was the file handle which was first opened. The check
> > may go wrong if the device was opened by multiple process closely enough in
> > time.
>
> I don't like this patch for a few reasons: first of all it makes open/close
> different from the open/close handling for normal v4l2 drivers. The decision
> was made to use a core lock to serialize ioctls, but not the file operations.
>
> The reason was that not all file operations need to take a lock, and that
> drivers often need to do special things in the open/close anyway and using
> the core lock for this caused more headaches than it solved.
>
> I think we need to stick to the same scheme here. Note that I wouldn't mind
> introducing a serialization lock for subdev ioctls. There isn't one at the
> moment, and I think that that will simplify locking for subdevs, just as it
> did for non-subdev drivers.
>
> The second problem is that this depends on a new flag which is fairly ugly.
>
> Drivers should just take a lock before calling fh_add and fh_singular.
The sub-device drivers cannot take the lock since the open/close handlers
often perform actions that themselves require serialisation. The
v4l2_subdev_fh is already initialised and added by the framework before the
driver has a chance to do anything.
>
> Things can be simplified a bit with the v4l2-fh functions: I think it makes
> sense if v4l2_fh_add and v4l2_fh_del both return a bool which is true if it
> was the first or last filehandle.
>
> That way you can just do:
>
> mutex_lock(&lock);
> if (v4l2_fh_add()) {
> ...
> }
> mutex_unlock(&lock);
>
> Same with v4l2_fh_del().
This does not work for sub-devices. For video devices it does.
>
> While we're at it: v4l2_fh_is_singular(_file) should return a bool, not an int.
Agreed. I think it was written in an era when people didn't think bool
existed. :-)
>
> Hmm, let me make a patch series for these fh changes, shouldn't be too difficult.
I'll review it.
I think the API change (return singularity from v4l2_fh_add() and
v4l2_fh_release()) is good IMO). I might even consider removing
v4l2_fh_is_singular() or at least deprecate it since it begs misuse in form
of completely ignoring proper serialisation, and it's also made redundant.
Also, your patchset does not address the problem on sub-device drivers.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
