On 04/12/2018 09:35 AM, Sakari Ailus wrote:
> Hi Tomasz,
>
> On Tue, Apr 10, 2018 at 08:59:23AM +0000, Tomasz Figa wrote:
>> Hi Hans,
>>
>> On Mon, Apr 9, 2018 at 11:21 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>>> Implement the MEDIA_REQUEST_IOC_QUEUE and MEDIA_REQUEST_IOC_REINIT
>>> ioctls.
>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>> ---
>>> drivers/media/media-request.c | 80
>> +++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 78 insertions(+), 2 deletions(-)
>>
>>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>>> index dffc290e4ada..27739ff7cb09 100644
>>> --- a/drivers/media/media-request.c
>>> +++ b/drivers/media/media-request.c
>>> @@ -118,10 +118,86 @@ static unsigned int media_request_poll(struct file
>> *filp,
>>> return 0;
>>> }
>>
>>> +static long media_request_ioctl_queue(struct media_request *req)
>>> +{
>>> + struct media_device *mdev = req->mdev;
>>> + unsigned long flags;
>>> + int ret = 0;
>>> +
>>> + dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
>>> +
>>> + spin_lock_irqsave(&req->lock, flags);
>>> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
>>> + dev_dbg(mdev->dev,
>>> + "request: unable to queue %s, request in state
>> %s\n",
>>> + req->debug_str,
>> media_request_state_str(req->state));
>>> + spin_unlock_irqrestore(&req->lock, flags);
>>> + return -EINVAL;
>>
>> nit: Perhaps -EBUSY? (vb2 returns -EINVAL, though, but IMHO it doesn't
>> really represent the real error too closely.)
>>
>>> + }
>>> + req->state = MEDIA_REQUEST_STATE_QUEUEING;
>>> +
>>> + spin_unlock_irqrestore(&req->lock, flags);
>>> +
>>> + /*
>>> + * Ensure the request that is validated will be the one that gets
>> queued
>>> + * next by serialising the queueing process.
>>> + */
>>> + mutex_lock(&mdev->req_queue_mutex);
>>> +
>>> + ret = mdev->ops->req_queue(req);
>>> + spin_lock_irqsave(&req->lock, flags);
>>> + req->state = ret ? MEDIA_REQUEST_STATE_IDLE :
>> MEDIA_REQUEST_STATE_QUEUED;
>>> + spin_unlock_irqrestore(&req->lock, flags);
>>> + mutex_unlock(&mdev->req_queue_mutex);
>>> +
>>> + if (ret) {
>>> + dev_dbg(mdev->dev, "request: can't queue %s (%d)\n",
>>> + req->debug_str, ret);
>>> + } else {
>>> + media_request_get(req);
>>
>> I'm not convinced that this is the right place to take a reference. IMHO
>> whoever saves a pointer to the request in its own internal data (the
>> ->req_queue() callback?), should also grab a reference before doing so. Not
>> a strong objection, though, if we clearly document this, so that whoever
>> implements ->req_queue() callback can do the right thing.
>
> The framework will also release that reference once the request is
> complete. In my opinion it's perfectly reasonable to do this in the
> framework; moving things to frameworks that do not need to be done in
> drivers generally reduces bugs and makes drivers more maintainable. Albeit
> this is a minor matter in this respect.
>
> Hans: the reference must be taken before the request is queued: otherwise
> it is possible that the driver completes the request before the reference
> is taken here. That would mean the request might have been already released
> by the time of getting a refenrece to it.
Actually, it can't. As long as userspace has the request fd open there is always
at least one reference. And since we're in an ioctl (thus using the fd) it can't
be closed yet.
Regards,
Hans
>
>>
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static long media_request_ioctl_reinit(struct media_request *req)
>>> +{
>>> + struct media_device *mdev = req->mdev;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&req->lock, flags);
>>> + if (req->state != MEDIA_REQUEST_STATE_IDLE &&
>>> + req->state != MEDIA_REQUEST_STATE_COMPLETE) {
>>> + dev_dbg(mdev->dev,
>>> + "request: %s not in idle or complete state,
>> cannot reinit\n",
>>> + req->debug_str);
>>> + spin_unlock_irqrestore(&req->lock, flags);
>>> + return -EINVAL;
>>
>> nit: Perhaps -EBUSY? (Again vb2 would return -EINVAL...)
>
> I agree on both return values.
>
