Re: [PATCH 05/13] vfio/fsl: Move to the device set infrastructure

[Jason Gunthorpe <jgg@nvidia.com>]

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..

> > -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);

Thanks,
Jason