On 28.01.2023 02:32, Stefano Stabellini wrote: > On Wed, 31 Aug 2022, Volodymyr Babchuk wrote: >> As pci devices are refcounted now and all list that store them are >> protected by separate locks, we can safely drop global pcidevs_lock. >> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com> > > Up until this patch this patch series introduces: > - d->pdevs_lock to protect d->pdev_list > - pci_seg->alldevs_lock to protect pci_seg->alldevs_list > - iommu->ats_list_lock to protect iommu->ats_devices > - pdev refcounting to detect a pdev in-use and when to free it > - pdev->lock to protect pdev->msi_list > > They cover a lot of ground. Are they collectively covering everything > pcidevs_lock() was protecting? > > deassign_device is not protected by pcidevs_lock anymore. > deassign_device accesses a number of pdev fields, including quarantine, > phantom_stride and fault.count. > > deassign_device could run at the same time as assign_device who sets > quarantine and other fields. > > It looks like assign_device, deassign_device, and other functions > accessing/modifying pdev fields should be protected by pdev->lock. > > In fact, I think it would be safer to make sure every place that > currently has a pcidevs_lock() gets a pdev->lock (unless there is a > d->pdevs_lock, pci_seg->alldevs_lock, iommu->ats_list_lock, or another > lock) ?
Yes, I agree - there shouldn't be cases where lock uses are removed with neither replacement nor an explanation why the removal is safe. Which in turn suggests that a change like the one here likely needs doing in much smaller chunks. Grouping could possibly be based upon all touched instances having the same replacement or justification. Jan
