On Sun, Jan 12, 2025 at 07:46:33PM +0100, Kirill A. Korinsky wrote:
> On Sun, 12 Jan 2025 17:28:49 +0100,
> Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
> > 
> > On Sun, Jan 12, 2025 at 01:28:09AM +0100, Kirill A. Korinsky wrote:
> > > tech@,
> > > 
> > > I'd like to sync V4L2 API to the latest Linux kernel. The last time it
> > > was synced with 4.10-rc6 in February 2017. 8 years is a long time.
> > > 
> > > It builds, but to avoid fallout, I think we need a test bulkd build.
> > > 
> > > Anyone willing to run it?
> > 
> > I have put this diff as-is in a bulk build but my fastest amd64
> > machine will probably need 3-4 days to complete it if it doesn't hit
> > stupid build failures.
> > 
> 
> Thanks! I really doubt that it will hit any issue, but who knows.
> 
> > > This is large diff and I did my best to replace all __u/__s, but some
> > > things migth be snitched.
> > > 
> > > Feedback? Ok?
> > 
> > I wonder whether defining stuff that we don't support might result in
> > runtime breakage.  When following the trail for VIDIOC_REMOVE_BUFS I
> > can see that it is documented as available if
> > V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS is listed as supported in the new
> > 'capabilities' field of v4l2_requestbuffers etc.  This field in turn
> > is documented as "Set by the driver. If 0, then the driver doesn’t
> > support capabilities."[0].
> > 
> > This lead to another question: should the structure changes be
> > audited so that we make sure we set the additional fields to
> > appropriate values (possibly zero)?
> >
> 
> Right now both V4L2 drivers uvideo and utvfu zeroes struct v4l2_capability
> and setup only required fields.
> 
> This logic should be quite safe, I think.

I was really talking about additional fields in (pre)existing structs.
See:

/* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */

I think the capabilities field in struct v4l2_requestbuffers should be
set (or zeroed?) by uvideo/utvfu.  Else an application that starts
picking up this field might see uninitialized data if it doesn't zero
the field before calling the ioctl.  I'm not seeing similar fields in
other preexisting structs (full list: v4l2_fmtdesc,
v4l2_requestbuffers, v4l2_buffer, v4l2_ext_controls;
v4l2_create_buffers is modified but VIDIOC_CREATE_BUFS isn't
implemented).

> > Just sharing my thoughts, I know next to nothing about the use of V4L2
> > API and its use in the ecosystem.
> > 
> 
> Thanks, I appriciete it, and this is quite large diff...
> 
> > Regarding the content of the diff itself, the #ifdef/#ifndef
> > __KERNEL__ chunks look wrong, the define we use is _KERNEL.  The
> > #ifdef ones should probably be dropped if we don't plan to use the
> > definitions they're hiding in our kernel.  For example struct
> > __kernel_v4l2_timeval (ugh) looks out of place on OpenBSD.  The
> > #ifndef chunks should probably be adapted to our #ifndef _KERNEL
> > idiom.
> >
> 
> The currect code contains 3 __KERNEL__ and 1 _KERNEL. My diff adds a few
> more, and replaces _KERNEL to __KERNEL__. I not sure that it is bad things,
> special if such macros is wrong. Yes, I may clean up this code, but it
> increases burden for future sync.
> 
> Not sure that it is a good idea.

I had not noticed the existing occurrences of #ifndef __KERNEL__.  The
actual name checked doesn't matter much technically since it's not
supposed to be defined when used from userland.  It looks weird and
inconsistent with the rest of our includes, but I don't feel too
strongly about those #ifndef.

The #ifdef __KERNEL__ bits are really ugly though.  It shouldn't be
too hard to skip them when diffing against the linux header.

-- 
jca