| Message ID | 20181019021132.14743-4-okaya@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v6,1/7] PCI: Expose reset_type to users of __pci_reset_function_locked() | expand |
[+cc mwifiex maintainers, Brian] On Fri, Oct 19, 2018 at 02:11:24AM +0000, Sinan Kaya wrote: > Looking to have more control between the users of the API vs. what the API > can do internally. The new reset_type tells the PCI core about the bounds > of the request. > > Signed-off-by: Sinan Kaya <okaya@kernel.org> > --- > drivers/pci/pci.c | 5 +++-- > drivers/vfio/pci/vfio_pci.c | 7 ++++--- > drivers/vfio/pci/vfio_pci_config.c | 4 ++-- > include/linux/pci.h | 2 +- Doesn't mwifiex_pcie_card_reset_work() also use pci_try_reset_function() and need an update? vfio is special in several ways, but I'm a little dubious about mwifiex since it's the only other driver in the whole tree that uses pci_try_reset_function(). The mwifiex use was added by a64e7a79dd60 ("mwifiex: resolve reset vs. remove()/shutdown() deadlocks"). > 4 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 9a649d1adb13..7739f28988ae 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4851,10 +4851,11 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked); > /** > * pci_try_reset_function - quiesce and reset a PCI device function > * @dev: PCI device to reset > + * @reset_type: reset type to apply > * > * Same as above, except return -EAGAIN if unable to lock device. > */ > -int pci_try_reset_function(struct pci_dev *dev) > +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type) > { > int rc; > > @@ -4865,7 +4866,7 @@ int pci_try_reset_function(struct pci_dev *dev) > return -EAGAIN; > > pci_dev_save_and_disable(dev); > - rc = __pci_reset_function_locked(dev, PCI_RESET_ANY); > + rc = __pci_reset_function_locked(dev, reset_type); > pci_dev_restore(dev); > pci_dev_unlock(dev); > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index cddb453a1ba5..fe7ada997c51 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -228,7 +228,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > return ret; > > /* If reset fails because of the device lock, fail this path entirely */ > - ret = pci_try_reset_function(pdev); > + ret = pci_try_reset_function(pdev, PCI_RESET_ANY); > if (ret == -EAGAIN) { > pci_disable_device(pdev); > return ret; > @@ -376,7 +376,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) > * 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 && !pci_try_reset_function(pdev)) > + if (vdev->reset_works && !pci_try_reset_function(pdev, PCI_RESET_ANY)) > vdev->needs_reset = false; > > pci_restore_state(pdev); > @@ -844,7 +844,8 @@ static long vfio_pci_ioctl(void *device_data, > > } else if (cmd == VFIO_DEVICE_RESET) { > return vdev->reset_works ? > - pci_try_reset_function(vdev->pdev) : -EINVAL; > + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY) : > + -EINVAL; > > } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) { > struct vfio_pci_hot_reset_info hdr; > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > index 115a36f6f403..0d66bac66211 100644 > --- a/drivers/vfio/pci/vfio_pci_config.c > +++ b/drivers/vfio/pci/vfio_pci_config.c > @@ -831,7 +831,7 @@ static int vfio_exp_config_write(struct vfio_pci_device *vdev, int pos, > &cap); > > if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) > - pci_try_reset_function(vdev->pdev); > + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY); > } > > /* > @@ -910,7 +910,7 @@ static int vfio_af_config_write(struct vfio_pci_device *vdev, int pos, > &cap); > > if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) > - pci_try_reset_function(vdev->pdev); > + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY); > } > > return count; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 9103ac1b3c31..cde63e0a85ea 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1170,7 +1170,7 @@ int pcie_flr(struct pci_dev *dev); > int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); > int pci_reset_function(struct pci_dev *dev, u32 reset_type); > int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); > -int pci_try_reset_function(struct pci_dev *dev); > +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type); > int pci_probe_reset_slot(struct pci_slot *slot); > int pci_probe_reset_bus(struct pci_bus *bus); > int pci_reset_bus(struct pci_dev *dev); > -- > 2.19.0 >
On Fri, Oct 19, 2018 at 1:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc mwifiex maintainers, Brian] > > On Fri, Oct 19, 2018 at 02:11:24AM +0000, Sinan Kaya wrote: > > Looking to have more control between the users of the API vs. what the API > > can do internally. The new reset_type tells the PCI core about the bounds > > of the request. > > > > Signed-off-by: Sinan Kaya <okaya@kernel.org> > > --- > > drivers/pci/pci.c | 5 +++-- > > drivers/vfio/pci/vfio_pci.c | 7 ++++--- > > drivers/vfio/pci/vfio_pci_config.c | 4 ++-- > > include/linux/pci.h | 2 +- > > Doesn't mwifiex_pcie_card_reset_work() also use > pci_try_reset_function() and need an update? Yes :) > vfio is special in several ways, but I'm a little dubious about > mwifiex since it's the only other driver in the whole tree that uses > pci_try_reset_function(). > > The mwifiex use was added by a64e7a79dd60 ("mwifiex: resolve reset vs. > remove()/shutdown() deadlocks"). I won't say that patch is perfect (there are cases where we'll end up failing to try to reset, and the device will just stay dead instead), but it definitely resolved deadlocks in a way that looked reasonable to me. Feel free to poke holes in my original commit description, or else ask questions, if you'd like mwifiex to do something differently. Brian > > 4 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 9a649d1adb13..7739f28988ae 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4851,10 +4851,11 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked); > > /** > > * pci_try_reset_function - quiesce and reset a PCI device function > > * @dev: PCI device to reset > > + * @reset_type: reset type to apply > > * > > * Same as above, except return -EAGAIN if unable to lock device. > > */ > > -int pci_try_reset_function(struct pci_dev *dev) > > +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type) > > { > > int rc; > > > > @@ -4865,7 +4866,7 @@ int pci_try_reset_function(struct pci_dev *dev) > > return -EAGAIN; > > > > pci_dev_save_and_disable(dev); > > - rc = __pci_reset_function_locked(dev, PCI_RESET_ANY); > > + rc = __pci_reset_function_locked(dev, reset_type); > > pci_dev_restore(dev); > > pci_dev_unlock(dev); > > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > > index cddb453a1ba5..fe7ada997c51 100644 > > --- a/drivers/vfio/pci/vfio_pci.c > > +++ b/drivers/vfio/pci/vfio_pci.c > > @@ -228,7 +228,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > > return ret; > > > > /* If reset fails because of the device lock, fail this path entirely */ > > - ret = pci_try_reset_function(pdev); > > + ret = pci_try_reset_function(pdev, PCI_RESET_ANY); > > if (ret == -EAGAIN) { > > pci_disable_device(pdev); > > return ret; > > @@ -376,7 +376,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) > > * 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 && !pci_try_reset_function(pdev)) > > + if (vdev->reset_works && !pci_try_reset_function(pdev, PCI_RESET_ANY)) > > vdev->needs_reset = false; > > > > pci_restore_state(pdev); > > @@ -844,7 +844,8 @@ static long vfio_pci_ioctl(void *device_data, > > > > } else if (cmd == VFIO_DEVICE_RESET) { > > return vdev->reset_works ? > > - pci_try_reset_function(vdev->pdev) : -EINVAL; > > + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY) : > > + -EINVAL; > > > > } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) { > > struct vfio_pci_hot_reset_info hdr; > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > index 115a36f6f403..0d66bac66211 100644 > > --- a/drivers/vfio/pci/vfio_pci_config.c > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > @@ -831,7 +831,7 @@ static int vfio_exp_config_write(struct vfio_pci_device *vdev, int pos, > > &cap); > > > > if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) > > - pci_try_reset_function(vdev->pdev); > > + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY); > > } > > > > /* > > @@ -910,7 +910,7 @@ static int vfio_af_config_write(struct vfio_pci_device *vdev, int pos, > > &cap); > > > > if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) > > - pci_try_reset_function(vdev->pdev); > > + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY); > > } > > > > return count; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 9103ac1b3c31..cde63e0a85ea 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -1170,7 +1170,7 @@ int pcie_flr(struct pci_dev *dev); > > int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); > > int pci_reset_function(struct pci_dev *dev, u32 reset_type); > > int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); > > -int pci_try_reset_function(struct pci_dev *dev); > > +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type); > > int pci_probe_reset_slot(struct pci_slot *slot); > > int pci_probe_reset_bus(struct pci_bus *bus); > > int pci_reset_bus(struct pci_dev *dev); > > -- > > 2.19.0 > >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9a649d1adb13..7739f28988ae 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4851,10 +4851,11 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked); /** * pci_try_reset_function - quiesce and reset a PCI device function * @dev: PCI device to reset + * @reset_type: reset type to apply * * Same as above, except return -EAGAIN if unable to lock device. */ -int pci_try_reset_function(struct pci_dev *dev) +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type) { int rc; @@ -4865,7 +4866,7 @@ int pci_try_reset_function(struct pci_dev *dev) return -EAGAIN; pci_dev_save_and_disable(dev); - rc = __pci_reset_function_locked(dev, PCI_RESET_ANY); + rc = __pci_reset_function_locked(dev, reset_type); pci_dev_restore(dev); pci_dev_unlock(dev); diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index cddb453a1ba5..fe7ada997c51 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -228,7 +228,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) return ret; /* If reset fails because of the device lock, fail this path entirely */ - ret = pci_try_reset_function(pdev); + ret = pci_try_reset_function(pdev, PCI_RESET_ANY); if (ret == -EAGAIN) { pci_disable_device(pdev); return ret; @@ -376,7 +376,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev) * 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 && !pci_try_reset_function(pdev)) + if (vdev->reset_works && !pci_try_reset_function(pdev, PCI_RESET_ANY)) vdev->needs_reset = false; pci_restore_state(pdev); @@ -844,7 +844,8 @@ static long vfio_pci_ioctl(void *device_data, } else if (cmd == VFIO_DEVICE_RESET) { return vdev->reset_works ? - pci_try_reset_function(vdev->pdev) : -EINVAL; + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY) : + -EINVAL; } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) { struct vfio_pci_hot_reset_info hdr; diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c index 115a36f6f403..0d66bac66211 100644 --- a/drivers/vfio/pci/vfio_pci_config.c +++ b/drivers/vfio/pci/vfio_pci_config.c @@ -831,7 +831,7 @@ static int vfio_exp_config_write(struct vfio_pci_device *vdev, int pos, &cap); if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) - pci_try_reset_function(vdev->pdev); + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY); } /* @@ -910,7 +910,7 @@ static int vfio_af_config_write(struct vfio_pci_device *vdev, int pos, &cap); if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) - pci_try_reset_function(vdev->pdev); + pci_try_reset_function(vdev->pdev, PCI_RESET_ANY); } return count; diff --git a/include/linux/pci.h b/include/linux/pci.h index 9103ac1b3c31..cde63e0a85ea 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1170,7 +1170,7 @@ int pcie_flr(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); int pci_reset_function(struct pci_dev *dev, u32 reset_type); int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); -int pci_try_reset_function(struct pci_dev *dev); +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type); int pci_probe_reset_slot(struct pci_slot *slot); int pci_probe_reset_bus(struct pci_bus *bus); int pci_reset_bus(struct pci_dev *dev);
Looking to have more control between the users of the API vs. what the API can do internally. The new reset_type tells the PCI core about the bounds of the request. Signed-off-by: Sinan Kaya <okaya@kernel.org> --- drivers/pci/pci.c | 5 +++-- drivers/vfio/pci/vfio_pci.c | 7 ++++--- drivers/vfio/pci/vfio_pci_config.c | 4 ++-- include/linux/pci.h | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-)