| Message ID | 20250113170724.801717-1-chenghaoyang@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, thank you for the patch. Harvey Yang <chenghaoyang@chromium.org> writes: > As DmaSyncer disables the copy c'tor, the move c'tor will be used > instead. This leaves some DmaSyncers with invalid SharedFDs. They should > avoid syncing with invalid file descriptors in the d'tor. > > Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable") > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> Tested-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/dma_buf_allocator.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > index a014c3b4263c..17a888efaa40 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -330,6 +330,11 @@ DmaSyncer::~DmaSyncer() > > void DmaSyncer::sync(uint64_t step) > { > + // DmaSyncer might be moved and left an empty SharedFD. ... left with an ... ? > + // Avoid syncing with an invalid file descriptor in this case. I think /* ... */ style comments are to be used in libcamera. > + if (!fd_.isValid()) > + return; > + > struct dma_buf_sync sync = { > .flags = flags_ | step > };
On Mon, Jan 13, 2025 at 05:06:56PM +0000, Harvey Yang wrote: > As DmaSyncer disables the copy c'tor, the move c'tor will be used > instead. This leaves some DmaSyncers with invalid SharedFDs. They should > avoid syncing with invalid file descriptors in the d'tor. > > Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable") > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > src/libcamera/dma_buf_allocator.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > index a014c3b4263c..17a888efaa40 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -330,6 +330,11 @@ DmaSyncer::~DmaSyncer() > > void DmaSyncer::sync(uint64_t step) > { > + // DmaSyncer might be moved and left an empty SharedFD. > + // Avoid syncing with an invalid file descriptor in this case. Comment style. > + if (!fd_.isValid()) > + return; I would move this to the destructor, as calling sync() with an invalid fd in the constructor is an error that should result in a loud log message. > + > struct dma_buf_sync sync = { > .flags = flags_ | step > };
Hi Laurent, On Mon, Jan 13, 2025 at 7:00 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Jan 13, 2025 at 05:06:56PM +0000, Harvey Yang wrote: > > As DmaSyncer disables the copy c'tor, the move c'tor will be used > > instead. This leaves some DmaSyncers with invalid SharedFDs. They should > > avoid syncing with invalid file descriptors in the d'tor. > > > > Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable") > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > src/libcamera/dma_buf_allocator.cpp | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > index a014c3b4263c..17a888efaa40 100644 > > --- a/src/libcamera/dma_buf_allocator.cpp > > +++ b/src/libcamera/dma_buf_allocator.cpp > > @@ -330,6 +330,11 @@ DmaSyncer::~DmaSyncer() > > > > void DmaSyncer::sync(uint64_t step) > > { > > + // DmaSyncer might be moved and left an empty SharedFD. > > + // Avoid syncing with an invalid file descriptor in this case. > > Comment style. Yeah sorry. Will be updated in the next version. > > > + if (!fd_.isValid()) > > + return; > > I would move this to the destructor, as calling sync() with an invalid > fd in the constructor is an error that should result in a loud log > message. Makes sense. Will be moved. BR, Harvey > > > + > > struct dma_buf_sync sync = { > > .flags = flags_ | step > > }; > > -- > Regards, > > Laurent Pinchart
Hi Milan, On Mon, Jan 13, 2025 at 7:00 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > Hi Harvey, > > thank you for the patch. > > Harvey Yang <chenghaoyang@chromium.org> writes: > > > As DmaSyncer disables the copy c'tor, the move c'tor will be used > > instead. This leaves some DmaSyncers with invalid SharedFDs. They should > > avoid syncing with invalid file descriptors in the d'tor. > > > > Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable") > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > Tested-by: Milan Zamazal <mzamazal@redhat.com> > > > --- > > src/libcamera/dma_buf_allocator.cpp | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > index a014c3b4263c..17a888efaa40 100644 > > --- a/src/libcamera/dma_buf_allocator.cpp > > +++ b/src/libcamera/dma_buf_allocator.cpp > > @@ -330,6 +330,11 @@ DmaSyncer::~DmaSyncer() > > > > void DmaSyncer::sync(uint64_t step) > > { > > + // DmaSyncer might be moved and left an empty SharedFD. > > ... left with an ... ? Done > > > + // Avoid syncing with an invalid file descriptor in this case. > > I think /* ... */ style comments are to be used in libcamera. Right, will be updated. BR, Harvey > > > + if (!fd_.isValid()) > > + return; > > + > > struct dma_buf_sync sync = { > > .flags = flags_ | step > > }; >
diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp index a014c3b4263c..17a888efaa40 100644 --- a/src/libcamera/dma_buf_allocator.cpp +++ b/src/libcamera/dma_buf_allocator.cpp @@ -330,6 +330,11 @@ DmaSyncer::~DmaSyncer() void DmaSyncer::sync(uint64_t step) { + // DmaSyncer might be moved and left an empty SharedFD. + // Avoid syncing with an invalid file descriptor in this case. + if (!fd_.isValid()) + return; + struct dma_buf_sync sync = { .flags = flags_ | step };
As DmaSyncer disables the copy c'tor, the move c'tor will be used instead. This leaves some DmaSyncers with invalid SharedFDs. They should avoid syncing with invalid file descriptors in the d'tor. Fixes: 545046a41e17 ("DmaBufAllocator: Make DmaSyncer non-copyable") Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- src/libcamera/dma_buf_allocator.cpp | 5 +++++ 1 file changed, 5 insertions(+)