| Message ID | 20140115033854.18253.62934.stgit@bling.home (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Jan 14, 2014 at 8:45 PM, Alex Williamson <alex.williamson@redhat.com> wrote: > PCI resets will attempt to take the device_lock for any device to be > reset. This is a problem if that lock is already held, for instance > in the device remove path. It's not sufficient to simply kill the > user process or skip the reset if called after .remove as a race could > result in the same deadlock. Instead, we handle all resets as "best > effort" using the PCI "try" reset interfaces. This prevents the user > from being able to induce a deadlock by triggering a reset. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > v2: Update error message to make it clear that device reset failed > and change back to pr_warn to make sure it's visible. I'll > address whether dev_warn would be more appropriate in future > updates. > > Bjorn, if this looks good and you don't mind, please take this in > through your tree so that it syncs with the introduction of the > pci_try_reset interfaces. Thanks! I applied this to pci/reset (along with the try_reset implementation) and pushed it to my "next" branch, thanks! > drivers/vfio/pci/vfio_pci.c | 29 +++++++++-------------------- > 1 file changed, 9 insertions(+), 20 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 6ab71b9..2319d20 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -139,25 +139,14 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) > pci_write_config_word(pdev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE); > > /* > - * Careful, device_lock may already be held. This is the case if > - * a driver unbind is blocked. Try to get the locks ourselves to > - * prevent a deadlock. > + * Try to reset the device. The success of this is dependent on > + * being able to lock the device, which is not always possible. > */ > if (vdev->reset_works) { > - bool reset_done = false; > - > - if (pci_cfg_access_trylock(pdev)) { > - if (device_trylock(&pdev->dev)) { > - __pci_reset_function_locked(pdev); > - reset_done = true; > - device_unlock(&pdev->dev); > - } > - pci_cfg_access_unlock(pdev); > - } > - > - if (!reset_done) > - pr_warn("%s: Unable to acquire locks for reset of %s\n", > - __func__, dev_name(&pdev->dev)); > + int ret = pci_try_reset_function(pdev); > + if (ret) > + pr_warn("%s: Failed to reset device %s (%d)\n", > + __func__, dev_name(&pdev->dev), ret); > } > > pci_restore_state(pdev); > @@ -514,7 +503,7 @@ static long vfio_pci_ioctl(void *device_data, > > } else if (cmd == VFIO_DEVICE_RESET) { > return vdev->reset_works ? > - pci_reset_function(vdev->pdev) : -EINVAL; > + pci_try_reset_function(vdev->pdev) : -EINVAL; > > } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) { > struct vfio_pci_hot_reset_info hdr; > @@ -684,8 +673,8 @@ reset_info_exit: > &info, slot); > if (!ret) > /* User has access, do the reset */ > - ret = slot ? pci_reset_slot(vdev->pdev->slot) : > - pci_reset_bus(vdev->pdev->bus); > + ret = slot ? pci_try_reset_slot(vdev->pdev->slot) : > + pci_try_reset_bus(vdev->pdev->bus); > > hot_reset_release: > for (i--; i >= 0; i--) > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 6ab71b9..2319d20 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -139,25 +139,14 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) pci_write_config_word(pdev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE); /* - * Careful, device_lock may already be held. This is the case if - * a driver unbind is blocked. Try to get the locks ourselves to - * prevent a deadlock. + * Try to reset the device. The success of this is dependent on + * being able to lock the device, which is not always possible. */ if (vdev->reset_works) { - bool reset_done = false; - - if (pci_cfg_access_trylock(pdev)) { - if (device_trylock(&pdev->dev)) { - __pci_reset_function_locked(pdev); - reset_done = true; - device_unlock(&pdev->dev); - } - pci_cfg_access_unlock(pdev); - } - - if (!reset_done) - pr_warn("%s: Unable to acquire locks for reset of %s\n", - __func__, dev_name(&pdev->dev)); + int ret = pci_try_reset_function(pdev); + if (ret) + pr_warn("%s: Failed to reset device %s (%d)\n", + __func__, dev_name(&pdev->dev), ret); } pci_restore_state(pdev); @@ -514,7 +503,7 @@ static long vfio_pci_ioctl(void *device_data, } else if (cmd == VFIO_DEVICE_RESET) { return vdev->reset_works ? - pci_reset_function(vdev->pdev) : -EINVAL; + pci_try_reset_function(vdev->pdev) : -EINVAL; } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) { struct vfio_pci_hot_reset_info hdr; @@ -684,8 +673,8 @@ reset_info_exit: &info, slot); if (!ret) /* User has access, do the reset */ - ret = slot ? pci_reset_slot(vdev->pdev->slot) : - pci_reset_bus(vdev->pdev->bus); + ret = slot ? pci_try_reset_slot(vdev->pdev->slot) : + pci_try_reset_bus(vdev->pdev->bus); hot_reset_release: for (i--; i >= 0; i--)
PCI resets will attempt to take the device_lock for any device to be reset. This is a problem if that lock is already held, for instance in the device remove path. It's not sufficient to simply kill the user process or skip the reset if called after .remove as a race could result in the same deadlock. Instead, we handle all resets as "best effort" using the PCI "try" reset interfaces. This prevents the user from being able to induce a deadlock by triggering a reset. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- v2: Update error message to make it clear that device reset failed and change back to pr_warn to make sure it's visible. I'll address whether dev_warn would be more appropriate in future updates. Bjorn, if this looks good and you don't mind, please take this in through your tree so that it syncs with the introduction of the pci_try_reset interfaces. Thanks! drivers/vfio/pci/vfio_pci.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html