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.
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().
While we're at it: v4l2_fh_is_singular(_file) should return a bool, not an int.
Hmm, let me make a patch series for these fh changes, shouldn't be too difficult.
Regards,
Hans
>
> Why this is especially important is because many drivers perform power
> management and other hardware initialisation based on open file handles.
>
> Example one: Two processes open the device, but both end up determining
> neither was the first file handle opened on the device.
>
> process A: v4l2_fh_add()
> process B: v4l2_fh_add()
>
> ...
>
> process A: v4l2_fh_is_singular() # false
> process B: v4l2_fh_is_singular() # false
>
> Example two:
>
> process A: v4l2_fh_add()
>
> ...
>
> process A: v4l2_fh_is_singular() # true
> # at this point the driver does
> # time-consuming hardware
> # initialisation in the context of
> # process A
>
> process B: v4l2_fh_add()
> process B: v4l2_fh_is_singular() # false
> # open system call finishes
>
> ...
>
> process B proceeds to access the sub-device
>
> ...
>
> process A finishes hardware initialisation
>
> If the sub-device's open() handler in struct v4l2_subdev_internal_ops is not
> defined, then the information on whether the file handle was the first to be
> opened is not needed to complete the open system call on the device, and
> serialisation of the open and release system calls thus is not needed, with
> the possible exception for the driver's own reasons, but that is entirely
> the job of the driver.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-device.c | 4 ++++
> drivers/media/v4l2-core/v4l2-subdev.c | 41 +++++++++++++++++++++++++++++------
> include/media/v4l2-subdev.h | 4 ++++
> 3 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 5b0a30b..a13e0be 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -191,6 +191,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
> }
> #endif
>
> + mutex_init(&sd->open_lock);
> +
> spin_lock(&v4l2_dev->lock);
> list_add_tail(&sd->list, &v4l2_dev->subdevs);
> spin_unlock(&v4l2_dev->lock);
> @@ -292,5 +294,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> video_unregister_device(sd->devnode);
> if (!sd->owner_v4l2_dev)
> module_put(sd->owner);
> +
> + mutex_destroy(&sd->open_lock);
> }
> EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b3f7da9..d8a98c4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -50,6 +50,18 @@ static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> #endif
> }
>
> +static void subdev_open_lock(struct v4l2_subdev *sd)
> +{
> + if (sd->flags & V4L2_SUBDEV_FL_SERIALISE_OPEN)
> + mutex_lock(&sd->open_lock);
> +}
> +
> +static void subdev_open_unlock(struct v4l2_subdev *sd)
> +{
> + if (sd->flags & V4L2_SUBDEV_FL_SERIALISE_OPEN)
> + mutex_unlock(&sd->open_lock);
> +}
> +
> static int subdev_open(struct file *file)
> {
> struct video_device *vdev = video_devdata(file);
> @@ -64,11 +76,11 @@ static int subdev_open(struct file *file)
> if (subdev_fh == NULL)
> return -ENOMEM;
>
> + subdev_open_lock(sd);
> +
> ret = subdev_fh_init(subdev_fh, sd);
> - if (ret) {
> - kfree(subdev_fh);
> - return ret;
> - }
> + if (ret)
> + goto err_free_subdev_fh;
>
> v4l2_fh_init(&subdev_fh->vfh, vdev);
> v4l2_fh_add(&subdev_fh->vfh);
> @@ -78,7 +90,7 @@ static int subdev_open(struct file *file)
> entity = media_entity_get(&sd->entity);
> if (!entity) {
> ret = -EBUSY;
> - goto err;
> + goto err_media_entity_put;
> }
> }
> #endif
> @@ -86,20 +98,26 @@ static int subdev_open(struct file *file)
> if (sd->internal_ops && sd->internal_ops->open) {
> ret = sd->internal_ops->open(sd, subdev_fh);
> if (ret < 0)
> - goto err;
> + goto err_media_entity_put;
> }
>
> + subdev_open_unlock(sd);
> +
> return 0;
>
> -err:
> +err_media_entity_put:
> #if defined(CONFIG_MEDIA_CONTROLLER)
> media_entity_put(entity);
> #endif
> v4l2_fh_del(&subdev_fh->vfh);
> v4l2_fh_exit(&subdev_fh->vfh);
> subdev_fh_free(subdev_fh);
> +
> +err_free_subdev_fh:
> kfree(subdev_fh);
>
> + subdev_open_unlock(sd);
> +
> return ret;
> }
>
> @@ -110,6 +128,8 @@ static int subdev_close(struct file *file)
> struct v4l2_fh *vfh = file->private_data;
> struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
>
> + subdev_open_lock(sd);
> +
> if (sd->internal_ops && sd->internal_ops->close)
> sd->internal_ops->close(sd, subdev_fh);
> #if defined(CONFIG_MEDIA_CONTROLLER)
> @@ -117,7 +137,11 @@ static int subdev_close(struct file *file)
> media_entity_put(&sd->entity);
> #endif
> v4l2_fh_del(vfh);
> +
> + subdev_open_unlock(sd);
> +
> v4l2_fh_exit(vfh);
> +
> subdev_fh_free(subdev_fh);
> kfree(subdev_fh);
> file->private_data = NULL;
> @@ -586,6 +610,9 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> sd->ops = ops;
> sd->v4l2_dev = NULL;
> sd->flags = 0;
> + /* Default to serialised open and release. */
> + if (sd->internal_ops && sd->internal_ops->open)
> + sd->flags |= V4L2_SUBDEV_FL_SERIALISE_OPEN;
> sd->name[0] = '\0';
> sd->grp_id = 0;
> sd->dev_priv = NULL;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 5622699..11ffd50 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -624,6 +624,8 @@ struct v4l2_subdev_internal_ops {
> #define V4L2_SUBDEV_FL_HAS_DEVNODE (1U << 2)
> /* Set this flag if this subdev generates events. */
> #define V4L2_SUBDEV_FL_HAS_EVENTS (1U << 3)
> +/* Set this flag if the sub-device open/close do NOT need to be serialised. */
> +#define V4L2_SUBDEV_FL_SERIALISE_OPEN (1U << 4)
>
> struct regulator_bulk_data;
>
> @@ -672,6 +674,8 @@ struct v4l2_subdev {
> struct v4l2_async_notifier *notifier;
> /* common part of subdevice platform data */
> struct v4l2_subdev_platform_data *pdata;
> + /* serialise open and release based on the flags field */
> + struct mutex open_lock;
> };
>
> #define media_entity_to_v4l2_subdev(ent) \
>
--
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
