Messages in this thread | | | Date | Wed, 10 Mar 2021 08:01:49 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCH v1 09/14] vfio/type1: Refactor pfn_list clearing |
| |
> +/* Return 1 if iommu->lock dropped and notified, 0 if done */
A bool would be more useful for that pattern.
> +static int unmap_dma_pfn_list(struct vfio_iommu *iommu, struct vfio_dma *dma, > + struct vfio_dma **dma_last, int *retries) > +{ > + if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
Just return early when it is empty to remove a level of indentation for the whole function?
> + struct vfio_iommu_type1_dma_unmap nb_unmap; > + > + if (*dma_last == dma) { > + BUG_ON(++(*retries) > 10); > + } else { > + *dma_last = dma; > + *retries = 0; > + } > + > + nb_unmap.iova = dma->iova; > + nb_unmap.size = dma->size; > + > + /* > + * Notify anyone (mdev vendor drivers) to invalidate and > + * unmap iovas within the range we're about to unmap. > + * Vendor drivers MUST unpin pages in response to an > + * invalidation. > + */ > + mutex_unlock(&iommu->lock); > + blocking_notifier_call_chain(&iommu->notifier, > + VFIO_IOMMU_NOTIFY_DMA_UNMAP, > + &nb_unmap); > + return 1;
Conditional locking is a horrible pattern. I'd rather only factor the code until before the unlock into the helper, and then leave the unlock and notify to the caller to avoid that anti-pattern.
Also vendor driver isn't really Linux terminology for a subdriver, so I'd suggest to switch to something else while you're at it.
| |