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
