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
