Re: [RFC v2 03/11] vfio: Set device->group in helper function

From: Yi Liu <yi.l.liu@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: <alex.williamson@redhat.com>, <kevin.tian@intel.com>,
	<eric.auger@redhat.com>, <cohuck@redhat.com>,
	<nicolinc@nvidia.com>, <yi.y.sun@linux.intel.com>,
	<chao.p.peng@linux.intel.com>, <mjrosato@linux.ibm.com>,
	<kvm@vger.kernel.org>
Subject: Re: [RFC v2 03/11] vfio: Set device->group in helper function
Date: Thu, 24 Nov 2022 21:50:14 +0800	[thread overview]
Message-ID: <fcb43043-0547-e368-f8a4-a396bf7c7390@intel.com> (raw)
In-Reply-To: <Y39u3446TXjcxkUz@nvidia.com>

On 2022/11/24 21:17, Jason Gunthorpe wrote:
> On Thu, Nov 24, 2022 at 04:26:54AM -0800, Yi Liu wrote:
>> This avoids referencing device->group in __vfio_register_dev()
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/vfio/vfio_main.c | 52 +++++++++++++++++++++++++---------------
>>   1 file changed, 33 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
>> index 7a78256a650e..4980b8acf5d3 100644
>> --- a/drivers/vfio/vfio_main.c
>> +++ b/drivers/vfio/vfio_main.c
>> @@ -503,10 +503,15 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>>   	return group;
>>   }
>>   
>> -static int __vfio_register_dev(struct vfio_device *device,
>> -		struct vfio_group *group)
>> +static int vfio_device_set_group(struct vfio_device *device,
>> +				 enum vfio_group_type type)
>>   {
>> -	int ret;
>> +	struct vfio_group *group;
>> +
>> +	if (type == VFIO_IOMMU)
>> +		group = vfio_group_find_or_alloc(device->dev);
>> +	else
>> +		group = vfio_noiommu_group_alloc(device->dev, type);
>>   
>>   	/*
>>   	 * In all cases group is the output of one of the group allocation
> 
> This comment should be deleted

ok.

>> @@ -515,6 +520,16 @@ static int __vfio_register_dev(struct vfio_device *device,
>>   	if (IS_ERR(group))
>>   		return PTR_ERR(group);
>>   
>> +	/* Our reference on group is moved to the device */
>> +	device->group = group;
>> +	return 0;
>> +}
>> +
>> +static int __vfio_register_dev(struct vfio_device *device,
>> +			       enum vfio_group_type type)
>> +{
>> +	int ret;
>> +
>>   	if (WARN_ON(device->ops->bind_iommufd &&
>>   		    (!device->ops->unbind_iommufd ||
>>   		     !device->ops->attach_ioas)))
>> @@ -527,34 +542,33 @@ static int __vfio_register_dev(struct vfio_device *device,
>>   	if (!device->dev_set)
>>   		vfio_assign_device_set(device, device);
>>   
>> -	/* Our reference on group is moved to the device */
>> -	device->group = group;
>> -
>>   	ret = dev_set_name(&device->device, "vfio%d", device->index);
>>   	if (ret)
>> -		goto err_out;
>> +		return ret;
>>   
>> -	ret = device_add(&device->device);
>> +	ret = vfio_device_set_group(device, type);
>>   	if (ret)
>> -		goto err_out;
>> +		return ret;
>> +
>> +	ret = device_add(&device->device);
>> +	if (ret) {
>> +		vfio_device_remove_group(device);
>> +		return ret;
> 
> You could probably keep the goto

after the change, only this branch will use the err_out; may be not 
necessary to use goto. but keep goto may have less changes in patch
file. e.g. I'm ok to keep goto.

@@ -527,12 +542,13 @@ static int __vfio_register_dev(struct vfio_device 
*device,
         if (!device->dev_set)
                 vfio_assign_device_set(device, device);

-       /* Our reference on group is moved to the device */
-       device->group = group;
-
         ret = dev_set_name(&device->device, "vfio%d", device->index);
         if (ret)
-               goto err_out;
+               return ret;
+
+       ret = vfio_device_set_group(device, type);
+       if (ret)
+               return ret;

         ret = device_add(&device->device);
         if (ret)


-- 
Regards,
Yi Liu
