| Message ID | 20221228213212.628636-1-shacharr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert backwards goto into a while loop | expand |
On Wed, 28 Dec 2022 21:32:12 +0000 Shachar Raindel <shacharr@google.com> wrote: > The function vaddr_get_pfns used a goto retry structure to implement > retrying. This is discouraged by the coding style guide (which is > only recommending goto for handling function exits). Convert the code > to a while loop, making it explicit that the follow block only runs > when the pin attempt failed. What coding style guide are you referring to? In Documentation/process/coding-style.rst I only see goto mentioned in 7) Centralized exiting of functions, which suggests it's a useful mechanism for creating centralized cleanup code, while noting "[a]lbeit deprecated by *some people*", emphasis added. This doesn't suggest to me such a strong statement as implied in this commit log. > Signed-off-by: Shachar Raindel <shacharr@google.com> > --- > drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c > b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00..7f38d7fc3f62 > 100644 --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm, > unsigned long vaddr, } > > *pfn = page_to_pfn(pages[0]); > - goto done; > - } > + } else Coding style would however discourage skipping the braces around this half of the branch, as done here, as a) they're used around the former half and b) the below is not a single simple statement. Thanks, Alex > + do { > + > + /* This is not a normal page, lookup PFN for P2P DMA */ > + vaddr = untagged_addr(vaddr); > > - vaddr = untagged_addr(vaddr); > + vma = vma_lookup(mm, vaddr); > > -retry: > - vma = vma_lookup(mm, vaddr); > + if (!vma || !(vma->vm_flags & VM_PFNMAP)) > + break; > > - if (vma && vma->vm_flags & VM_PFNMAP) { > - ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE); > - if (ret == -EAGAIN) > - goto retry; > + ret = follow_fault_pfn(vma, mm, vaddr, pfn, > + prot & IOMMU_WRITE); > + if (ret) > + continue; /* Retry for EAGAIN, otherwise bail */ > > - if (!ret) { > if (is_invalid_reserved_pfn(*pfn)) > ret = 1; > else > ret = -EFAULT; > - } > - } > -done: > + } while (ret == -EAGAIN); > + > mmap_read_unlock(mm); > return ret; > }
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00..7f38d7fc3f62 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm, unsigned long vaddr, } *pfn = page_to_pfn(pages[0]); - goto done; - } + } else + do { + + /* This is not a normal page, lookup PFN for P2P DMA */ + vaddr = untagged_addr(vaddr); - vaddr = untagged_addr(vaddr); + vma = vma_lookup(mm, vaddr); -retry: - vma = vma_lookup(mm, vaddr); + if (!vma || !(vma->vm_flags & VM_PFNMAP)) + break; - if (vma && vma->vm_flags & VM_PFNMAP) { - ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE); - if (ret == -EAGAIN) - goto retry; + ret = follow_fault_pfn(vma, mm, vaddr, pfn, + prot & IOMMU_WRITE); + if (ret) + continue; /* Retry for EAGAIN, otherwise bail */ - if (!ret) { if (is_invalid_reserved_pfn(*pfn)) ret = 1; else ret = -EFAULT; - } - } -done: + } while (ret == -EAGAIN); + mmap_read_unlock(mm); return ret; }
The function vaddr_get_pfns used a goto retry structure to implement retrying. This is discouraged by the coding style guide (which is only recommending goto for handling function exits). Convert the code to a while loop, making it explicit that the follow block only runs when the pin attempt failed. Signed-off-by: Shachar Raindel <shacharr@google.com> --- drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)