On 08.11.22 05:45, Tomasz Figa wrote:
Hi David,
Hi Tomasz,
thanks for looking into this!
On Tue, Nov 8, 2022 at 1:19 AM David Hildenbrand david@redhat.com wrote:
FOLL_FORCE is really only for debugger access. According to commit 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are always writable"), the pinned pages are always writable.
As reference, the cover letter of the series:
https://lkml.kernel.org/r/20221107161740.144456-1-david@redhat.com
Actually that patch is only a workaround to temporarily disable support for read-only pages as they seemed to suffer from some corruption issues in the retrieved user pages. We expect to support read-only pages as hardware input after. That said, FOLL_FORCE doesn't sound like the right thing even in that case, but I don't know the background behind it being added here in the first place. +Hans Verkuil +Marek Szyprowski do you happen to remember anything about it?
Maybe I mis-interpreted 707947247e95; re-reading it again, I am not quite sure what the actual problem is and how it relates to GUP FOLL_WRITE handling. FOLL_FORCE already was in place before 707947247e95 and should be removed.
What I understood is "Just always call vb2_create_framevec() with FOLL_WRITE to always allow writing to the buffers.".
If the pinned page is never written to via the obtained GUP reference: * FOLL_WRITE should not be set * FOLL_FORCE should not be set * We should not dirty the page when unpinning
If the pinned page may be written to via the obtained GUP reference: * FOLL_WRITE should be set * FOLL_FORCE should not be set * We should dirty the page when unpinning
If the function is called for both, we should think about doing it conditional based on a "write" variable, like pre-707947247e95 did.
@Hans, any insight?