On 18/10/2023 23:39, Jason Gunthorpe wrote: > On Wed, Oct 18, 2023 at 09:27:04PM +0100, Joao Martins wrote: > >> +int iommufd_check_iova_range(struct iommufd_ioas *ioas, >> + struct iommu_hwpt_get_dirty_iova *bitmap) >> +{ >> + unsigned long pgshift, npages; >> + size_t iommu_pgsize; >> + int rc = -EINVAL; >> + >> + pgshift = __ffs(bitmap->page_size); >> + npages = bitmap->length >> pgshift; > > npages = bitmap->length / bitmap->page_size; > > ? (if page_size is a bitmask it is badly named) > It was a way to avoid the divide by zero, but I can switch to the above, and check for bitmap->page_size being non-zero. should be less obscure >> +static int __iommu_read_and_clear_dirty(struct iova_bitmap *bitmap, >> + unsigned long iova, size_t length, >> + void *opaque) >> +{ >> + struct iopt_area *area; >> + struct iopt_area_contig_iter iter; >> + struct iova_bitmap_fn_arg *arg = opaque; >> + struct iommu_domain *domain = arg->domain; >> + struct iommu_dirty_bitmap *dirty = arg->dirty; >> + const struct iommu_dirty_ops *ops = domain->dirty_ops; >> + unsigned long last_iova = iova + length - 1; >> + int ret = -EINVAL; >> + >> + iopt_for_each_contig_area(&iter, area, arg->iopt, iova, last_iova) { >> + unsigned long last = min(last_iova, iopt_area_last_iova(area)); >> + >> + ret = ops->read_and_clear_dirty(domain, iter.cur_iova, >> + last - iter.cur_iova + 1, >> + 0, dirty); > > This seems like a lot of stuff going on with ret.. > All to have a single return exit point, given no different cleanup is required. Thought it was the general best way (when possible) >> + if (ret) > > return ret > >> + break; >> + } >> + >> + if (!iopt_area_contig_done(&iter)) >> + ret = -EINVAL; > > return -EINVAL > >> + >> + return ret; > > return 0; > > And remove the -EINVAL. OK > iopt_area_contig_done() captures the case > where the iova range is not fully contained by areas, even the case > where there are no areas. > > But otherwise > > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx> > > Jason