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] [day] [month] [year] [list]

Date:   Mon, 4 Sep 2023 12:49:46 +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 reply.

> > 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?
>
> Ah, I don't think I have ever seen anyone use buf_finish in that way!
>
> Very inventive, and perfectly legal. Actually a very nice idea!
>
> So, with that in mind there are still some things that need to change.
>
> First of all, you can drop the 'VOLATILE' flag from the control, instead
> just call v4l2_ctrl_s_ctrl() from buf_finish() to update the control.
> And in stop_streaming the control value should probably be set to 0.
>
> The use of volatile for a control is a last resort, and in this case it
> is not volatile at all.
>
> Secondly, this behavior has to be documented: in buf_finish add a comment
> along the lines of: "This callback is called when the buffer is dequeued,
> so update this control with the number of rectangles."
>
> And where the control is defined, refer to buf_finish to explain where it
> is set.
>
> Finally the user-facing documentation has to be updated (npcm-video.rst)
> to explain this behavior.

OK. Will drop the VOLATILE flag and update comment/document in the next version.

Regards,
Marvin