On Tue, Jun 07, 2022 at 07:44:37AM +0200, Christoph Hellwig wrote: > On Mon, Jun 06, 2022 at 09:34:36PM -0300, Jason Gunthorpe wrote: > > + if (!list_empty(&iommu->device_list)) { > > + mutex_lock(&iommu->device_list_lock); > > + mutex_unlock(&iommu->lock); > > + > > + list_for_each_entry(device, > > + &iommu->device_list, > > + iommu_entry) > > + device->ops->dma_unmap( > > + device, dma->iova, dma->size); > > + > > + mutex_unlock(&iommu->device_list_lock); > > + mutex_lock(&iommu->lock); > > + } > > I wonder if factoring this into a little helper instead of the > very deep indentation might be a bit better for readability. > > > +static void vfio_iommu_type1_register_device(void *iommu_data, > > + struct vfio_device *vdev) > > { > > struct vfio_iommu *iommu = iommu_data; > > > > + if (!vdev->ops->dma_unmap) > > + return; > > > > + mutex_lock(&iommu->lock); > > + mutex_lock(&iommu->device_list_lock); > > + list_add(&vdev->iommu_entry, &iommu->device_list); > > + mutex_unlock(&iommu->device_list_lock); > > + mutex_unlock(&iommu->lock); > > Why do we need both iommu->lock and the device_list_lock everywhere? Not everwhere, all the readers are using only one of the locks. The list empty calls that were previously unlocked are done under the iommu->lock and only the list iteration was done under the device_list. > Maybe explain the locking scheme somewhere so that people don't have > to guess, because it seems to me that just using iommu->lock would > be enough right now. The expectation is that the dma_umap callback will re-enter the type1 driver via vfio_unpin_pages calls and this will recurse back onto the iommu->lock - so it must be dropped before invoking the callback. I'll add a note Jason