[Intel-gfx] [PATCH 05/13] vfio/fsl: Move to the device set infrastructure

[Diana Craciun OSS]

On 7/20/2021 7:17 PM, Jason Gunthorpe wrote:
> On Tue, Jul 20, 2021 at 07:12:26PM +0300, Diana Craciun OSS wrote:
>> On 7/15/2021 3:20 AM, Jason Gunthorpe wrote:
>>> FSL uses the internal reflck to implement the open_device() functionality,
>>> conversion to the core code is straightforward.
>>>
>>> The decision on which set to be part of is trivially based on the
>>> is_fsl_mc_bus_dprc() and we use a 'struct device *' pointer as the set_id.
>>>
>>> It isn't entirely clear what the device set lock is actually protecting,
>>> but I think it is related to the interrupt setup.
>>
>> Yes, it is protecting the interrupts setup. The FSL MC devices are using
>> MSIs and only the DPRC device is allocating the MSIs from the MSI domain.
>> The other devices just take interrupts from a pool. The lock is protecting
>> the access to this pool.
> 
> It would be much clearer if the lock was near the data it was
> protecting, the DPRC pool seems in an entirely different layer..

Yes, I agree. I will think about of a more clearer design for a future 
improvement.

> 
>>> -static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
>>> +static void vfio_fsl_mc_close_device(struct vfio_device *core_vdev)
>>>    {
>>>    	struct vfio_fsl_mc_device *vdev =
>>>    		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>>> +	struct fsl_mc_device *mc_dev = vdev->mc_dev;
>>> +	struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
>>> +	struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
>>>    	int ret;
>>> -	mutex_lock(&vdev->reflck->lock);
>>> +	vfio_fsl_mc_regions_cleanup(vdev);
>>> -	if (!(--vdev->refcnt)) {
>>> -		struct fsl_mc_device *mc_dev = vdev->mc_dev;
>>> -		struct device *cont_dev = fsl_mc_cont_dev(&mc_dev->dev);
>>> -		struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
>>> -
>>> -		vfio_fsl_mc_regions_cleanup(vdev);
>>> +	/* reset the device before cleaning up the interrupts */
>>> +	ret = dprc_reset_container(mc_cont->mc_io, 0, mc_cont->mc_handle,
>>> +				   mc_cont->obj_desc.id,
>>> +				   DPRC_RESET_OPTION_NON_RECURSIVE);
>>> -		/* reset the device before cleaning up the interrupts */
>>> -		ret = dprc_reset_container(mc_cont->mc_io, 0,
>>> -		      mc_cont->mc_handle,
>>> -			  mc_cont->obj_desc.id,
>>> -			  DPRC_RESET_OPTION_NON_RECURSIVE);
>>> +	if (WARN_ON(ret))
>>> +		dev_warn(&mc_cont->dev,
>>> +			 "VFIO_FLS_MC: reset device has failed (%d)\n", ret);
>>> -		if (ret) {
>>> -			dev_warn(&mc_cont->dev, "VFIO_FLS_MC: reset device has failed (%d)\n",
>>> -				 ret);
>>> -			WARN_ON(1);
>>> -		}
>>> +	vfio_fsl_mc_irqs_cleanup(vdev);
>>> -		vfio_fsl_mc_irqs_cleanup(vdev);
>>> -
>>> -		fsl_mc_cleanup_irq_pool(mc_cont);
>>
>> There is also a need for the lock here. Eventhough the close function is
>> called only once, there might be a race between the devices in the
>> set.
> 
> vfio_fsl_mc_close_device() is already called under this lock:
> 
> 	mutex_lock(&device->dev_set->lock);
> 	if (!--device->open_count && device->ops->close_device)
> 		device->ops->close_device(device);
> 	mutex_unlock(&device->dev_set->lock);
> 

OK, I missed that.

> Thanks,
> Jason
> 

I have tested the changes and everything works as expected.

Thanks,
Diana