On 8/24/22 18:24, Christian König wrote: > Am 24.08.22 um 12:22 schrieb Dmitry Osipenko: >> Move dma-buf attachment API functions to the dynamic locking >> specification. >> The strict locking convention prevents deadlock situations for dma-buf >> importers and exporters. >> >> Previously, the "unlocked" versions of the attachment API functions >> weren't taking the reservation lock and this patch makes them to take >> the lock. >> >> Intel and AMD GPU drivers already were mapping the attached dma-bufs >> under >> the held lock during attachment, hence these drivers are updated to use >> the locked functions. >> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> >> --- >> drivers/dma-buf/dma-buf.c | 115 ++++++++++++++------- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- >> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 8 +- >> drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +++ >> include/linux/dma-buf.h | 20 ++-- >> 5 files changed, 110 insertions(+), 49 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 4556a12bd741..f2a5a122da4a 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf >> *dmabuf, int flags) >> * 2. Userspace passes this file-descriptors to all drivers it wants >> this buffer >> * to share with: First the file descriptor is converted to a >> &dma_buf using >> * dma_buf_get(). Then the buffer is attached to the device using >> - * dma_buf_attach(). >> + * dma_buf_attach_unlocked(). > > Now I get why this is confusing me so much. > > The _unlocked postfix implies that there is another function which > should be called with the locks already held, but this is not the case > for attach/detach (because they always need to grab the lock themselves). That's correct. The attach/detach ops of exporter can take the lock (like i915 exporter does it), hence importer must not grab the lock around dma_buf_attach() invocation. > So I suggest to drop the _unlocked postfix for the attach/detach > functions. Another step would then be to unify attach/detach with > dynamic_attach/dynamic_detach when both have the same locking convention > anyway. It's not a problem to change the name, but it's unclear to me why we should do it. The _unlocked postfix tells importer that reservation must be unlocked and it must be unlocked in case of dma_buf_attach(). Dropping the postfix will make dma_buf_attach() inconsistent with the rest of the _unlocked functions(?). Are you sure we need to rename it? > Sorry that this is going so much back and forth, it's really complicated > to keep all the stuff in my head at the moment :) Not a problem at all, I expected that it will take some time for this patchset to settle down. -- Best regards, Dmitry
