| Message ID | 20240802072416.25297-2-umang.jain@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Quoting Umang Jain (2024-08-02 08:24:15) > V4L userspace API documentation clearly mentions the handling of > EINTR on ioctl(). Align with the xioctl() reference provided in [1]. > > Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{} > to check the return value of ::ioctl() and errno value. If the return > value is found to be -1, and errno is set with EINTR, the ioctl() call > should be retried. > > [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html > I haven't checked where, but I presume any of the operations that can be long running or blocking in the kernel such as DQ_BUF would have interruptible locks, which would make this important to have. Because we use poll/select to only call those when we know there will be something available, I expect that the chance of hitting this is incredibly low, - but I do believe it's more correct to ensure we handle this. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/v4l2_device.cpp | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 4a2048cf..2e92db0f 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -454,11 +454,17 @@ int V4L2Device::setFrameStartEnabled(bool enable) > */ > int V4L2Device::ioctl(unsigned long request, void *argp) > { > + int ret; > + > + do { > + ret = ::ioctl(fd_.get(), request, argp); > + } while (ret == -1 && errno == EINTR); > + > /* > * Printing out an error message is usually better performed > * in the caller, which can provide more context. > */ > - if (::ioctl(fd_.get(), request, argp) < 0) > + if (ret < 0) > return -errno; > > return 0; > -- > 2.45.2 >
Hi Umang, Thank you for the patch. On Fri, Aug 02, 2024 at 12:54:15PM +0530, Umang Jain wrote: > V4L userspace API documentation clearly mentions the handling of > EINTR on ioctl(). Align with the xioctl() reference provided in [1]. I'll need a better rationale, cargo-cult programming isn't a good enough explanation. > Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{} > to check the return value of ::ioctl() and errno value. If the return > value is found to be -1, and errno is set with EINTR, the ioctl() call > should be retried. > > [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/v4l2_device.cpp | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 4a2048cf..2e92db0f 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -454,11 +454,17 @@ int V4L2Device::setFrameStartEnabled(bool enable) > */ > int V4L2Device::ioctl(unsigned long request, void *argp) > { > + int ret; > + > + do { > + ret = ::ioctl(fd_.get(), request, argp); > + } while (ret == -1 && errno == EINTR); > + > /* > * Printing out an error message is usually better performed > * in the caller, which can provide more context. > */ > - if (::ioctl(fd_.get(), request, argp) < 0) > + if (ret < 0) > return -errno; > > return 0;
On Fri, Aug 02, 2024 at 08:40:50AM +0100, Kieran Bingham wrote: > Hi Umang, > > Quoting Umang Jain (2024-08-02 08:24:15) > > V4L userspace API documentation clearly mentions the handling of > > EINTR on ioctl(). Align with the xioctl() reference provided in [1]. > > > > Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{} > > to check the return value of ::ioctl() and errno value. If the return > > value is found to be -1, and errno is set with EINTR, the ioctl() call > > should be retried. > > > > [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html > > > > I haven't checked where, but I presume any of the operations that can be > long running or blocking in the kernel such as DQ_BUF would have > interruptible locks, which would make this important to have. > > Because we use poll/select to only call those when we know there will be > something available, I expect that the chance of hitting this is > incredibly low, - but I do believe it's more correct to ensure we handle > this. Another piece of information that is missing from the commit message and the cover letter is how this series came to be, and if it fixes a problem that has been noticed. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> The implementation is too much of a hack for my taste. The coments on 2/2 asking for factoring code out apply here too. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/libcamera/v4l2_device.cpp | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 4a2048cf..2e92db0f 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -454,11 +454,17 @@ int V4L2Device::setFrameStartEnabled(bool enable) > > */ > > int V4L2Device::ioctl(unsigned long request, void *argp) > > { > > + int ret; > > + > > + do { > > + ret = ::ioctl(fd_.get(), request, argp); > > + } while (ret == -1 && errno == EINTR); > > + > > /* > > * Printing out an error message is usually better performed > > * in the caller, which can provide more context. > > */ > > - if (::ioctl(fd_.get(), request, argp) < 0) > > + if (ret < 0) > > return -errno; > > > > return 0;
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 4a2048cf..2e92db0f 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -454,11 +454,17 @@ int V4L2Device::setFrameStartEnabled(bool enable) */ int V4L2Device::ioctl(unsigned long request, void *argp) { + int ret; + + do { + ret = ::ioctl(fd_.get(), request, argp); + } while (ret == -1 && errno == EINTR); + /* * Printing out an error message is usually better performed * in the caller, which can provide more context. */ - if (::ioctl(fd_.get(), request, argp) < 0) + if (ret < 0) return -errno; return 0;
V4L userspace API documentation clearly mentions the handling of EINTR on ioctl(). Align with the xioctl() reference provided in [1]. Retry the ::ioctl() if failed with errno == EINTR. Use a do..while{} to check the return value of ::ioctl() and errno value. If the return value is found to be -1, and errno is set with EINTR, the ioctl() call should be retried. [1]: https://docs.kernel.org/userspace-api/media/v4l/capture.c.html Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/v4l2_device.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)