On Thu, Jul 15, 2021 at 03:00:55PM -0600, Alex Williamson wrote:
> On Wed, 14 Jul 2021 21:20:38 -0300
> Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> > +/*
> > + * We need to get memory_lock for each device, but devices can share mmap_lock,
> > + * therefore we need to zap and hold the vma_lock for each device, and only then
> > + * get each memory_lock.
> > + */
> > +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev,
> > + struct vfio_pci_group_info *groups)
> > +{
> > + struct vfio_device_set *dev_set = vdev->vdev.dev_set;
> > + struct vfio_pci_device *cur_mem =
> > + list_first_entry(&dev_set->device_list, struct vfio_pci_device,
> > + vdev.dev_set_list);
>
> We shouldn't be looking at the list outside of the lock, if the first
> entry got removed we'd break our unwind code.
>
> > + struct vfio_pci_device *cur_vma;
> > + struct vfio_pci_device *cur;
> > + bool is_mem = true;
> > + int ret;
> >
> > - if (pci_dev_driver(pdev) != &vfio_pci_driver) {
> > - vfio_device_put(device);
> > - return -EBUSY;
> > + mutex_lock(&dev_set->lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Oh, righto, this is an oopsie!
> > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list)
> > + up_write(&cur->memory_lock);
> > + mutex_unlock(&dev_set->lock);
> > +
> > + return ret;
>
>
> Isn't the above section actually redundant to below, ie. we could just
> fall through after the pci_reset_bus()? Thanks,
It could, but I thought it was less confusing this way due to how
oddball the below is:
> > +err_undo:
> > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) {
> > + if (cur == cur_mem)
> > + is_mem = false;
> > + if (cur == cur_vma)
> > + break;
> > + if (is_mem)
> > + up_write(&cur->memory_lock);
> > + else
> > + mutex_unlock(&cur->vma_lock);
> > + }
But either works, do want it switch in v2?
Thanks,
Jason
