[PATCH 1/2] libcamera: v4l2_device: Retry ::ioctl on EINTR

[Laurent Pinchart]

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 at 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 at 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;

-- 
Regards,

Laurent Pinchart