On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> On Fri, Jan 12, 2018 at 8:37 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Support the request argument of the QBUF ioctl.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 93 +++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 92 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 8d041247e97f..28f9c368563e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -29,6 +29,7 @@
>>> #include <media/v4l2-device.h>
>>> #include <media/videobuf2-v4l2.h>
>>> #include <media/v4l2-mc.h>
>>> +#include <media/media-request.h>
>>>
>>> #include <trace/events/v4l2.h>
>>>
>>> @@ -965,6 +966,81 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
>>> return -EINVAL;
>>> }
>>>
>>> +/*
>>> + * Validate that a given request can be used during an ioctl.
>>> + *
>>> + * When using the request API, request file descriptors must be matched against
>>> + * the actual request object. User-space can pass any file descriptor, so we
>>> + * need to make sure the call is valid before going further.
>>> + *
>>> + * This function looks up the request and associated data and performs the
>>> + * following sanity checks:
>>> + *
>>> + * * Make sure that the entity supports requests,
>>> + * * Make sure that the entity belongs to the media_device managing the passed
>>> + * request,
>>> + * * Make sure that the entity data (if any) is associated to the current file
>>> + * handler.
>>> + *
>>> + * This function returns a pointer to the valid request, or and error code in
>>> + * case of failure. When successful, a reference to the request is acquired and
>>> + * must be properly released.
>>> + */
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> +static struct media_request *
>>> +check_request(int request, struct file *file, void *fh)
>>> +{
>>> + struct media_request *req = NULL;
>>> + struct video_device *vfd = video_devdata(file);
>>> + struct v4l2_fh *vfh =
>>> + test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>>> + struct media_entity *entity = &vfd->entity;
>>> + const struct media_entity *ent;
>>> + struct media_request_entity_data *data;
>>> + bool found = false;
>>> +
>>> + if (!entity)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + /* Check that the entity supports requests */
>>> + if (!entity->req_ops)
>>> + return ERR_PTR(-ENOTSUPP);
>>> +
>>> + req = media_request_get_from_fd(request);
>>
>> You can get the media_device from vfd->v4l2_dev->mdev. So it is much easier
>> to just pass the media_device as an argument to media_request_get_from_fd()...
>>
>>> + if (!req)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + /* Validate that the entity belongs to the media_device managing
>>> + * the request queue */
>>> + media_device_for_each_entity(ent, req->queue->mdev) {
>>> + if (entity == ent) {
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> + if (!found) {
>>> + media_request_put(req);
>>> + return ERR_PTR(-EINVAL);
>>> + }
>>
>> ...and then you don't need to do this ^^^ extra validation check.
>
> Ah right, all you need to do is check that req->queue->mdev ==
> vfd->v4l2_dev->mdev and you can get rid of this whole block.
Correct.
I don't
> think we can do that in media_request_get_from_fd() though, since it
> is called from other places where (IIUC) we don't have access to the
> media_device.
Yes, sorry about that. Ignore my comment about media_request_get_from_fd().
I misunderstood what that function did.
>
>>
>>> +
>>> + /* Validate that the entity's data belongs to the correct fh */
>>> + data = media_request_get_entity_data(req, entity, vfh);
>>> + if (IS_ERR(data)) {
>>> + media_request_put(req);
>>> + return ERR_PTR(PTR_ERR(data));
>>> + }
>>
>> This assumes that each filehandle has its own state. That's true for codecs,
>> but not for most (all?) other devices. There the state is per device instance.
>>
>> I'm not sure if we have a unique identifying mark for such drivers. The closest
>> is checking if fh->m2m_ctx is non-NULL, but I don't know if all drivers with
>> per-filehandle state use that field. An alternative might be to check if
>> fh->ctrl_handler is non-NULL. But again, I'm not sure if that's a 100% valid
>> check.
>
> I think the current code already takes that case into account: if the
> device does not uses v4l2_fh, then the fh argument passed to
> media_request_get_entity_data() will be null, and so will be data->fh.
>
Using v4l2_fh doesn't mean it has per-filehandle state. Most drivers only have global
state and they still use v4l2_fh.
This is why you should also look at vivid and vimc for the request API otherwise you
miss what non-codec drivers do. And those are the vast bulk of the drivers.
Regards,
Hans
