linux-kernel - Re: [PATCH v14 7/7] media: nuvoton: Add driver for NPCM video capture and encode engine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADnNmFqVbRWs5Uf_tJdrM0AopF_CmSHYwBK8-+1go_K6Hefkcw@mail.gmail.com>
Date:   Thu, 31 Aug 2023 20:42:38 +0800
From:   Kun-Fa Lin <milkfafa@...il.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     mchehab@...nel.org, avifishman70@...il.com, tmaimon77@...il.com,
        tali.perry1@...il.com, venture@...gle.com, yuenn@...gle.com,
        benjaminfair@...gle.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, andrzej.p@...labora.com,
        devicetree@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
        kwliu@...oton.com, kflin@...oton.com
Subject: Re: [PATCH v14 7/7] media: nuvoton: Add driver for NPCM video capture
 and encode engine

Hi Hans,

Thanks for the review.

> > +     snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", DEVICE_NAME);
>
> You can drop this last line, it is already filled in by the V4L2 core for
> platform devices.

> > +static const char * const npcm_ctrl_capture_mode_menu[] = {
> > +     "COMPLETE mode",
> > +     "DIFF mode",
>
> Hmm, I would drop the 'mode' bit, since it is already obvious that
> these are the modes.

OK. Will drop them in the next version.

> > +static const struct v4l2_ctrl_config npcm_ctrl_rect_count = {
> > +     .ops = &npcm_video_ctrl_ops,
> > +     .id = V4L2_CID_NPCM_RECT_COUNT,
> > +     .name = "NPCM Compressed Hextile Rectangle Count",
> > +     .type = V4L2_CTRL_TYPE_INTEGER,
> > +     .flags = V4L2_CTRL_FLAG_VOLATILE,
> > +     .min = 0,
> > +     .max = (MAX_WIDTH / RECT_W) * (MAX_HEIGHT / RECT_H),
> > +     .step = 1,
> > +     .def = 0,
> > +};
>
> Just to confirm: you decided against using an integer array control?
>
> There is a real danger that if userspace isn't reading this control
> quickly enough (i.e. before the next frame arrives at the driver), then
> the control's value is that of that next frame instead of the current
> frame.
>
> It doesn't feel robust to me.

Actually the driver will store the frames and counts for each buffer
index till userspace dequeues them.

Ex. assume that driver has captured 3 frames:
- 1st capture (buffer index = 0):
     video->list[0] => store the list of HEXTILE rects for the 1st frame
     video->rect[0] => store the rect count of video->list[0]
- 2nd capture (buffer index = 1):
     video->list[1] => store the list of HEXTILE rects for the 2nd frame
     video->rect[1] => store the rect count of video->list[1]
- 3rd capture (buffer index = 2):
     video->list[2] => store the list of HEXTILE rects for the 3rd frame
     video->rect[2] => store the rect count of video->list[2]

When userspace dequeues the 1st buffer (video->list[0]), it needs to
know the count of HEXTILE rectangles in the buffer,
so after dequeuing the buffer it will call this control to get the
rect count (video->rect[0]). And when a buffer is dequeued,
npcm_video_buf_finish() will be called, in which the buffer index (in
this example, buffer index = 0) will be stored to video->vb_index.
Then when userspace calls this control, npcm_video_get_volatile_ctrl()
will return the rect count of vb_index = 0.
In this way, I think userspace is always reading the correct control's
value even if userspace is slow.
Does it make sense to you or is there anything I missed?

Regards,
Marvin