On Tue, Aug 24, 2021 at 04:46:39PM +0200, Christoph Hellwig wrote:
> Factor out a helper to find or allocate the vfio_group to reduce the
> spagetthi code in vfio_register_group_dev a little.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> drivers/vfio/vfio.c | 49 ++++++++++++++++++++++++++-------------------
> 1 file changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 00aeef5bb29abd..9e97ad36a1c052 100644
> +++ b/drivers/vfio/vfio.c
> @@ -833,10 +833,32 @@ void vfio_uninit_group_dev(struct vfio_device *device)
> }
> EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
>
> +struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
> +{
> + struct iommu_group *iommu_group;
> + struct vfio_group *group;
> +
> + iommu_group = vfio_iommu_group_get(dev);
> + if (!iommu_group)
> + return ERR_PTR(-EINVAL);
> +
> + /*
> + * A found vfio_group already holds a reference to the iommu_group.
> + * A created vfio_group keeps the reference.
> + */
> + group = vfio_group_get_from_iommu(iommu_group);
> + if (!group) {
> + group = vfio_create_group(iommu_group);
> + if (!IS_ERR(group))
> + return group;
> + }
> + vfio_iommu_group_put(iommu_group, dev);
> + return group;
I think the non-"success oriented flow" is less readable than what was
before. It is jarring to see, and I was about to say this is not
logically the same having missed the !...
How about:
/* If found group already holds the iommu_group reference */
group = vfio_group_get_from_iommu(iommu_group);
if (group)
goto out_put;
group = vfio_create_group(iommu_group);
if (IS_ERR(group))
goto out_put;
/* If created our iommu_group reference was moved to group, keep it */
return group;
out_put:
vfio_iommu_group_put(iommu_group, dev);
return group;
Jason
