lkml.org 
[lkml]   [2021]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2021-03-10 09:03    [W:1.996 / U:0.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site