On 08/30/2018 12:15 PM, Sakari Ailus wrote: > Hi Hans, > > Thanks a lot for working on this! > > On Tue, Aug 28, 2018 at 03:49:10PM +0200, Hans Verkuil wrote: >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx> >> >> If requests are not supported by the driver, then return EACCES, not >> EPERM. This is consistent with the error that an invalid request_fd will >> give, and if requests are not supported, then all request_fd values are >> invalid. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> >> --- >> Documentation/media/uapi/v4l/buffer.rst | 2 +- >> .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 9 ++++----- >> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 15 +++++++++------ >> drivers/media/media-request.c | 4 ++-- >> 4 files changed, 16 insertions(+), 14 deletions(-) >> >> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst >> index 1865cd5b9d3c..58a6d7d336e6 100644 >> --- a/Documentation/media/uapi/v4l/buffer.rst >> +++ b/Documentation/media/uapi/v4l/buffer.rst >> @@ -314,7 +314,7 @@ struct v4l2_buffer >> :ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls. >> Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls >> other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`. >> - If the device does not support requests, then ``EPERM`` will be returned. >> + If the device does not support requests, then ``EACCES`` will be returned. >> If requests are supported but an invalid request file descriptor is >> given, then ``EINVAL`` will be returned. >> >> diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst >> index ad8908ce3095..54a999df5aec 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst >> @@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``, >> then the controls are not applied immediately when calling >> :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by >> the driver for the buffer associated with the same request. >> -If the device does not support requests, then ``EPERM`` will be returned. >> +If the device does not support requests, then ``EACCES`` will be returned. >> If requests are supported but an invalid request file descriptor is given, >> then ``EINVAL`` will be returned. >> >> @@ -233,7 +233,7 @@ still cause this situation. >> these controls have to be retrieved from a request or tried/set for >> a request. In the latter case the ``request_fd`` field contains the >> file descriptor of the request that should be used. If the device >> - does not support requests, then ``EPERM`` will be returned. >> + does not support requests, then ``EACCES`` will be returned. >> >> .. note:: >> >> @@ -299,7 +299,7 @@ still cause this situation. >> - ``request_fd`` >> - File descriptor of the request to be used by this operation. Only >> valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``. >> - If the device does not support requests, then ``EPERM`` will be returned. >> + If the device does not support requests, then ``EACCES`` will be returned. >> If requests are supported but an invalid request file descriptor is >> given, then ``EINVAL`` will be returned. >> * - __u32 >> @@ -408,6 +408,5 @@ EACCES >> control, or to get a control from a request that has not yet been >> completed. >> >> -EPERM > > -EACCES here, too? The '-' in -EPERM is from diff, meaning: delete this line :-) > >> - The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the >> + Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the >> device does not support requests. >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst >> index 7bff69c15452..a2f4ac0b0ba1 100644 >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst >> @@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver >> until the request itself is queued. Also, the driver will apply any >> settings associated with the request for this buffer. This field will >> be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set. >> -If the device does not support requests, then ``EPERM`` will be returned. >> +If the device does not support requests, then ``EACCES`` will be returned. >> If requests are supported but an invalid request file descriptor is given, >> then ``EINVAL`` will be returned. >> >> @@ -175,9 +175,12 @@ EPIPE >> codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already >> dequeued and no new buffers are expected to become available. >> >> -EPERM >> +EACCES >> The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not >> - support requests. Or the first buffer was queued via a request, but >> - the application now tries to queue it directly, or vice versa (it is >> - not permitted to mix the two APIs). Or an attempt is made to queue a >> - CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`. >> + support requests. >> + >> +EPERM >> + The first buffer was queued via a request, but the application now tries >> + to queue it directly, or vice versa (it is not permitted to mix the two >> + APIs). Or an attempt is made to queue a CAPTURE buffer to a request for a >> + :ref:`memory-to-memory device <codec>`. > > This is still apparently not quite the error code it should be --- EPERM is > about lacking permissions, not that the user did something that isn't > possible. We should not use an error code that has a well established > meaning everywhere else in uAPI already for a purpose that is very > different. > > If you think this needs to be something else than EACCES (which I think is > perfectly fine), then how about EDOM or EBUSY? Hmm. EPERM is returned for two reasons: - attempting to queue a buffer when you need to use requests, or vice versa. EBUSY would be a much better error code since the device is busy streaming in a different mode than you requested. And after you stop streaming you can use the requested mode again. - attempting to queue a request for a vb2 queue that doesn't support requests. This should return -EACCES. What do you think? Regards, Hans > >> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c >> index 414197645e09..4e9db1fed697 100644 >> --- a/drivers/media/media-request.c >> +++ b/drivers/media/media-request.c >> @@ -249,7 +249,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd) >> >> if (!mdev || !mdev->ops || >> !mdev->ops->req_validate || !mdev->ops->req_queue) >> - return ERR_PTR(-EPERM); >> + return ERR_PTR(-EACCES); >> >> filp = fget(request_fd); >> if (!filp) >> @@ -405,7 +405,7 @@ int media_request_object_bind(struct media_request *req, >> int ret = -EBUSY; >> >> if (WARN_ON(!ops->release)) >> - return -EPERM; >> + return -EACCES; >> >> spin_lock_irqsave(&req->lock, flags); >> >
