PCI: Add reset type parameter to PCI reset functions
Subsystems (11)
v4 cover PCI: Add reset type parameter to PCI reset functions
We want more control over what kind of reset PCI core can apply. Have the application explicitly requested allowed reset types.
differences from v3:
- set default return value to -EINVAL in __pci_reset_function_locked()
Sinan Kaya (6):
PCI: Expose reset_type to users of __pci_reset_function_locked()
PCI: Expose reset_type to users of pci_reset_function()
PCI: Expose reset_type to users of pci_reset_function_locked()
PCI: Expose reset type to users of pci_try_reset_function()
PCI: Expose reset type to users of pci_probe_reset_function()
PCI: Expose reset type to users of pci_reset_bus()
drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- drivers/misc/genwqe/card_base.c | 2 +- .../net/ethernet/cavium/liquidio/lio_main.c | 2 +- .../net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 2 +- drivers/net/ethernet/sfc/mcdi.c | 2 +- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/pci.c | 134 ++++++++++++------ drivers/pci/pci.h | 2 +- drivers/pci/probe.c | 2 +- drivers/vfio/pci/vfio_pci.c | 13 +- drivers/vfio/pci/vfio_pci_config.c | 4 +- drivers/xen/xen-pciback/pci_stub.c | 6 +- include/linux/pci.h | 21 ++- 13 files changed, 128 insertions(+), 66 deletions(-)
Signature · 1 line
2.18.0
PATCH v4 1/6 PCI: Expose reset_type to users of __pci_reset_function_locked()
We need a contract between the reset API users and the PCI core about the types of reset that a user needs vs. what PCI core can do internally. If a platform supports hotplug, we need to do hotplug reset as an example.
Expose the reset types to the drivers and try different reset types based on the new reset_type parameter.
Most users are expected to use PCI_RESET_ANY, PCI_RESET_FUNC or PCI_RESET_LINK parameters.
--- .../net/ethernet/cavium/liquidio/lio_main.c | 2 +- drivers/pci/pci.c | 59 ++++++++++++------- drivers/xen/xen-pciback/pci_stub.c | 6 +- include/linux/pci.h | 13 +++- 4 files changed, 55 insertions(+), 25 deletions(-)
drivers/net/ethernet/cavium/liquidio/lio_main.c +1 -1
@@ -989,7 +989,7 @@ static void octeon_pci_flr(struct octeon_device *oct)pci_write_config_word (oct->pci_dev,PCI_COMMAND ,PCI_COMMAND_INTX_DISABLE ); - rc =__pci_reset_function_locked (oct->pci_dev); + rc =__pci_reset_function_locked (oct->pci_dev,PCI_RESET_ANY );if (rc !=0 ) dev_err(&oct->pci_dev->dev, "Error %d resetting PCI function %d\n",
drivers/pci/pci.c +39 -20
99 lines not on this page · raw message
drivers/xen/xen-pciback/pci_stub.c +3 -3
@@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)/* Call the reset function which does not take lock as this * is called from "unbind" which takes a device_lock mutex. */ -__pci_reset_function_locked (dev); +__pci_reset_function_locked (dev,PCI_RESET_ANY );if (pci_load_and_free_saved_state (dev, &dev_data->pci_saved_state))dev_info (&dev->dev,"Could not reload PCI state\n" );else
@@ -283,7 +283,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)* (so it's ready for the next domain) */ device_lock_assert (&dev->dev); -__pci_reset_function_locked (dev); +__pci_reset_function_locked (dev,PCI_RESET_ANY ); dev_data =pci_get_drvdata (dev); ret =pci_load_saved_state (dev, dev_data->pci_saved_state);
@@ -417,7 +417,7 @@ static int pcistub_init_device(struct pci_dev *dev)dev_err (&dev->dev,"Could not store PCI conf saved state!\n" ); else {dev_dbg (&dev->dev,"resetting (FLR, D3, etc) the device\n" ); -__pci_reset_function_locked (dev); +__pci_reset_function_locked (dev,PCI_RESET_ANY );pci_restore_state (dev); }/* Now disable the device (this also ensures some private device
include/linux/pci.h +12 -1
@@ -849,6 +849,17 @@ enum { PCI_SCAN_ALL_PCIE_DEVS = 0x00000040, /* Scan all, not just dev 0 */ }; +#define PCI_RESET_DEV_SPECIFIC (1 << 0) +#define PCI_RESET_FLR (1 << 1) +#define PCI_RESET_PM (1 << 2) +#define PCI_RESET_SLOT (1 << 3) +#define PCI_RESET_BUS (1 << 4) + +#define PCI_RESET_ANY (~0) +#define PCI_RESET_FUNC (PCI_RESET_DEV_SPECIFIC | \ + PCI_RESET_FLR | PCI_RESET_PM) +#define PCI_RESET_LINK (PCI_RESET_SLOT | PCI_RESET_BUS) +/* These external functions are only available when PCI support is enabled */ #ifdef CONFIG_PCI
@@ -1111,7 +1122,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,void pcie_print_link_status (struct pci_dev *dev);bool pcie_has_flr (struct pci_dev *dev);int pcie_flr (struct pci_dev *dev); -int __pci_reset_function_locked (struct pci_dev *dev); +int __pci_reset_function_locked (struct pci_dev *dev,u32 reset_type);int pci_reset_function (struct pci_dev *dev);int pci_reset_function_locked (struct pci_dev *dev);int pci_try_reset_function (struct pci_dev *dev);
Signature · 1 line
2.18.0
Re: [PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()
Quoted 9 lines · Sinan Kaya
We need a contract between the reset API users and the PCI core about the types of reset that a user needs vs. what PCI core can do internally. If a platform supports hotplug, we need to do hotplug reset as an example.
Expose the reset types to the drivers and try different reset types based on the new
reset_typeparameter.Most users are expected to use
PCI_RESET_ANY,PCI_RESET_FUNCorPCI_RESET_LINKparameters.
I understand the words above, but I don't really understand the motivation behind this. Can you help flesh that out a bit?
I guess there's a problem being solved by these patches, and it would help a lot to have some details about what it is.
It would be ideal if we could offer some guidance, probably as code comments, to help driver writers choose the correct reset type.
drivers/pci/pci.c int __pci_reset_function_locked(struct pci_dev *dev) · line 4718
Quoted 7 lines
drivers/pci/pci.c +7
-0+if (reset_type &PCI_RESET_FLR ) { +if (pcie_has_flr (dev)) { + rc =pcie_flr (dev); +if (rc != -ENOTTY ) +return rc; + } + rc =pci_af_flr (dev,0 );
This is a tangent and not material for the current series, so just file it away so as not to delay this series, but is there any way we can get rid of the "probe" arguments? Maybe do all the probes at enumeration-time and remember the results?
I know there are one or two cases where you need to look at more than one device, e.g., a secondary bus reset might be possible if there's only one device and not otherwise, but it seems like in most cases this is a property of the individual device.
Bjorn
Re: [PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()
Quoted 9 lines · Bjorn Helgaas
Quoted 9 lines
We need a contract between the reset API users and the PCI core about the types of reset that a user needs vs. what PCI core can do internally. If a platform supports hotplug, we need to do hotplug reset as an example.
Expose the reset types to the drivers and try different reset types based on the new
reset_typeparameter.Most users are expected to use
PCI_RESET_ANY,PCI_RESET_FUNCorPCI_RESET_LINKparameters.
Quoted 9 lines
I understand the words above, but I don't really understand the motivation behind this. Can you help flesh that out a bit?
I guess there's a problem being solved by these patches, and it would help a lot to have some details about what it is.
It would be ideal if we could offer some guidance, probably as code comments, to help driver writers choose the correct reset type.
Alex and I discussed this here:
Re: [PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()
Quoted 16 lines · Sinan Kaya
Quoted 15 lines · Bjorn Helgaas
Quoted 9 lines
We need a contract between the reset API users and the PCI core about the types of reset that a user needs vs. what PCI core can do internally. If a platform supports hotplug, we need to do hotplug reset as an example.
Expose the reset types to the drivers and try different reset types based on the new
reset_typeparameter.Most users are expected to use
PCI_RESET_ANY,PCI_RESET_FUNCorPCI_RESET_LINKparameters.I understand the words above, but I don't really understand the motivation behind this. Can you help flesh that out a bit?
I guess there's a problem being solved by these patches, and it would help a lot to have some details about what it is.
Quoted 6 lines
Quoted 2 lines
It would be ideal if we could offer some guidance, probably as code comments, to help driver writers choose the correct reset type.
Alex and I discussed this here:
Thanks; needs to be distilled and included in the code as comments.
PATCH v4 2/6 PCI: Expose reset_type to users of pci_reset_function()
In reply to Sinan KayaLooking 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.
--- drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- drivers/misc/genwqe/card_base.c | 2 +- drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 2 +- drivers/net/ethernet/sfc/mcdi.c | 2 +- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/pci.c | 4 ++-- include/linux/pci.h | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-)
drivers/hid/intel-ish-hid/ipc/ipc.c +1 -1
@@ -754,7 +754,7 @@ static int _ish_hw_reset(struct ishtp_device *dev)if (!pdev)return -ENODEV ; - rv =pci_reset_function (pdev); + rv =pci_reset_function (pdev,PCI_RESET_ANY );if (!rv) dev->dev_state =ISHTP_DEV_RESETTING ;
drivers/misc/genwqe/card_base.c +1 -1
@@ -201,7 +201,7 @@ static int genwqe_bus_reset(struct genwqe_dev *cd)* restored by the pci_reset_function(). */ dev_dbg (&pci_dev->dev,"[%s] pci_reset function ...\n" , __func__); - rc =pci_reset_function (pci_dev); + rc =pci_reset_function (pci_dev,PCI_RESET_ANY );if (rc) {dev_err (&pci_dev->dev,"[%s] err: failed reset func (rc %d)\n" , __func__, rc);
drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c +1 -1
@@ -629,7 +629,7 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev)int i, err, ring;if (dev->flags &QLCNIC_NEED_FLR ) { -pci_reset_function (dev->pdev); +pci_reset_function (dev->pdev,PCI_RESET_ANY ); dev->flags &= ~QLCNIC_NEED_FLR ; }
drivers/net/ethernet/sfc/mcdi.c +1 -1
@@ -1862,7 +1862,7 @@ int efx_mcdi_reset(struct efx_nic *efx, enum reset_type method)/* If MCDI is down, we can't handle_assertion */ if (method ==RESET_TYPE_MCDI_TIMEOUT ) { - rc =pci_reset_function (efx->pci_dev); + rc =pci_reset_function (efx->pci_dev,PCI_RESET_ANY );if (rc)return rc;/* Re-enable polled MCDI completion */
drivers/pci/pci-sysfs.c +1 -1
@@ -1449,7 +1449,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,return -EINVAL ;pm_runtime_get_sync (dev); - result =pci_reset_function (pdev); + result =pci_reset_function (pdev,PCI_RESET_ANY );pm_runtime_put (dev);if (result <0 )return result;
drivers/pci/pci.c +2 -2
@@ -4793,7 +4793,7 @@ int pci_probe_reset_function(struct pci_dev *dev)* Returns 0 if the device function was successfully reset or negative if the * device doesn't support resetting a single function. */ -int pci_reset_function (struct pci_dev *dev) +int pci_reset_function (struct pci_dev *dev,u32 reset_type) {int rc;
@@ -4803,7 +4803,7 @@ int pci_reset_function(struct pci_dev *dev)pci_dev_lock (dev);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);
include/linux/pci.h +1 -1
@@ -1123,7 +1123,7 @@ void pcie_print_link_status(struct pci_dev *dev);bool pcie_has_flr (struct pci_dev *dev);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); +int pci_reset_function (struct pci_dev *dev,u32 reset_type);int pci_reset_function_locked (struct pci_dev *dev);int pci_try_reset_function (struct pci_dev *dev);int pci_probe_reset_slot (struct pci_slot *slot);
Signature · 1 line
2.18.0
Re: [PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()
Quoted 3 lines · Sinan Kaya
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.
I'd like to know why you chose PCI_RESET_ANY for these drivers.
Quoted 115 lines
Signed-off-by Sinan Kaya <[email protected]>--- drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- drivers/misc/genwqe/card_base.c | 2 +- drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 2 +- drivers/net/ethernet/sfc/mcdi.c | 2 +- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/pci.c | 4 ++-- include/linux/pci.h | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-)drivers/hid/intel-ish-hid/ipc/ipc.c +1
-1@@ -754,7 +754,7 @@ static int _ish_hw_reset(struct ishtp_device *dev)if (!pdev)return -ENODEV ; - rv =pci_reset_function (pdev); + rv =pci_reset_function (pdev,PCI_RESET_ANY );if (!rv) dev->dev_state =ISHTP_DEV_RESETTING ;drivers/misc/genwqe/card_base.c +1
-1@@ -201,7 +201,7 @@ static int genwqe_bus_reset(struct genwqe_dev *cd)* restored by the pci_reset_function(). */ dev_dbg (&pci_dev->dev,"[%s] pci_reset function ...\n" , __func__); - rc =pci_reset_function (pci_dev); + rc =pci_reset_function (pci_dev,PCI_RESET_ANY );if (rc) {dev_err (&pci_dev->dev,"[%s] err: failed reset func (rc %d)\n" , __func__, rc);drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c +1
-1@@ -629,7 +629,7 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev)int i, err, ring;if (dev->flags &QLCNIC_NEED_FLR ) { -pci_reset_function (dev->pdev); +pci_reset_function (dev->pdev,PCI_RESET_ANY ); dev->flags &= ~QLCNIC_NEED_FLR ; }drivers/net/ethernet/sfc/mcdi.c +1
-1@@ -1862,7 +1862,7 @@ int efx_mcdi_reset(struct efx_nic *efx, enum reset_type method)/* If MCDI is down, we can't handle_assertion */ if (method ==RESET_TYPE_MCDI_TIMEOUT ) { - rc =pci_reset_function (efx->pci_dev); + rc =pci_reset_function (efx->pci_dev,PCI_RESET_ANY );if (rc)return rc;/* Re-enable polled MCDI completion */ drivers/pci/pci-sysfs.c +1
-1@@ -1449,7 +1449,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,return -EINVAL ;pm_runtime_get_sync (dev); - result =pci_reset_function (pdev); + result =pci_reset_function (pdev,PCI_RESET_ANY );pm_runtime_put (dev);if (result <0 )return result;drivers/pci/pci.c +2
-2@@ -4793,7 +4793,7 @@ int pci_probe_reset_function(struct pci_dev *dev)* Returns 0 if the device function was successfully reset or negative if the * device doesn't support resetting a single function. */ -int pci_reset_function (struct pci_dev *dev) +int pci_reset_function (struct pci_dev *dev,u32 reset_type) {int rc;@@ -4803,7 +4803,7 @@ int pci_reset_function(struct pci_dev *dev)pci_dev_lock (dev);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);include/linux/pci.h +1
-1@@ -1123,7 +1123,7 @@ void pcie_print_link_status(struct pci_dev *dev);bool pcie_has_flr (struct pci_dev *dev);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); +int pci_reset_function (struct pci_dev *dev,u32 reset_type);int pci_reset_function_locked (struct pci_dev *dev);int pci_try_reset_function (struct pci_dev *dev);int pci_probe_reset_slot (struct pci_slot *slot);Signature · 1 line
2.18.0
Re: [PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()
Quoted 5 lines · Bjorn Helgaas
Quoted 3 lines
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.I'd like to know why you chose
PCI_RESET_ANYfor these drivers.
PCI_RESET_ANY was chosen to match the existing behavior in the code.
Current pci_reset_function() tries all reset types one by one until it returns 0. By specifying PCI_RESET_ANY, we get the same thing.
Re: [PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()
Quoted 11 lines · Sinan Kaya
Quoted 5 lines · Bjorn Helgaas
Quoted 3 lines
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.I'd like to know why you chose
PCI_RESET_ANYfor these drivers.
PCI_RESET_ANYwas chosen to match the existing behavior in the code.Current
pci_reset_function()tries all reset types one by one until it returns 0. By specifyingPCI_RESET_ANY, we get the same thing.
That would be useful information for the changelog.
PATCH v4 3/6 PCI: Expose reset_type to users of pci_reset_function_locked()
In reply to Sinan KayaLooking 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.
--- drivers/pci/pci.c | 4 ++-- include/linux/pci.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/pci/pci.c +2 -2
@@ -4829,7 +4829,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function);* Returns 0 if the device function was successfully reset or negative if the * device doesn't support resetting a single function. */ -int pci_reset_function_locked (struct pci_dev *dev) +int pci_reset_function_locked (struct pci_dev *dev,u32 reset_type) {int rc;
@@ -4838,7 +4838,7 @@ int pci_reset_function_locked(struct pci_dev *dev)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);
include/linux/pci.h +1 -1
@@ -1124,7 +1124,7 @@ bool pcie_has_flr(struct pci_dev *dev);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); +int pci_reset_function_locked (struct pci_dev *dev,u32 reset_type);int pci_try_reset_function (struct pci_dev *dev);int pci_probe_reset_slot (struct pci_slot *slot);int pci_probe_reset_bus (struct pci_bus *bus);
Signature · 1 line
2.18.0
PATCH v4 4/6 PCI: Expose reset type to users of pci_try_reset_function()
In reply to Sinan KayaLooking 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.
--- drivers/pci/pci.c | 4 ++-- drivers/vfio/pci/vfio_pci.c | 7 ++++--- drivers/vfio/pci/vfio_pci_config.c | 4 ++-- include/linux/pci.h | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-)
drivers/pci/pci.c +2 -2
@@ -4852,7 +4852,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);* * 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;
@@ -4863,7 +4863,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);
drivers/vfio/pci/vfio_pci.c +4 -3
@@ -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;
drivers/vfio/pci/vfio_pci_config.c +2 -2
@@ -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;
include/linux/pci.h +1 -1
@@ -1125,7 +1125,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);
Signature · 1 line
2.18.0
Re: [PATCH v4 4/6] PCI: Expose reset type to users of pci_try_reset_function()
Quoted 11 lines · On Wed, 19 Sep 2018 16:30:35 +0000 Sinan Kaya <[email protected]> wrote:
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 4 ++-- drivers/vfio/pci/vfio_pci.c | 7 ++++--- drivers/vfio/pci/vfio_pci_config.c | 4 ++-- include/linux/pci.h | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-)
Quoted 89 lines
drivers/pci/pci.c +2
-2@@ -4852,7 +4852,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);* * 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;@@ -4863,7 +4863,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);drivers/vfio/pci/vfio_pci.c +4
-3@@ -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;drivers/vfio/pci/vfio_pci_config.c +2
-2@@ -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;include/linux/pci.h +1
-1@@ -1125,7 +1125,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);
PATCH v4 5/6 PCI: Expose reset type to users of pci_probe_reset_function()
In reply to Sinan KayaLooking 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.
--- drivers/pci/pci.c | 51 ++++++++++++++++++++++++++++++--------------- drivers/pci/pci.h | 2 +- drivers/pci/probe.c | 2 +- 3 files changed, 36 insertions(+), 19 deletions(-)
drivers/pci/pci.c +34 -17
@@ -4753,28 +4753,45 @@ EXPORT_SYMBOL_GPL(__pci_reset_function_locked);* Returns 0 if the device function can be reset or negative if the * device doesn't support resetting a single function. */ -int pci_probe_reset_function (struct pci_dev *dev) +int pci_probe_reset_function (struct pci_dev *dev,u32 reset_type) { -int rc; +int rc =0 ;might_sleep (); - rc =pci_dev_specific_reset (dev,1 ); -if (rc != -ENOTTY ) -return rc; -if (pcie_has_flr (dev)) -return 0 ; - rc =pci_af_flr (dev,1 ); -if (rc != -ENOTTY ) -return rc; - rc =pci_pm_reset (dev,1 ); -if (rc != -ENOTTY ) -return rc; - rc =pci_dev_reset_slot_function (dev,1 ); -if (rc != -ENOTTY ) -return rc; +if (reset_type &PCI_RESET_DEV_SPECIFIC ) { + rc =pci_dev_specific_reset (dev,1 ); +if (rc != -ENOTTY ) +return rc; + } + +if (reset_type &PCI_RESET_FLR ) { +if (pcie_has_flr (dev)) +return 0 ; + rc =pci_af_flr (dev,1 ); +if (rc != -ENOTTY ) +return rc; + } -return pci_parent_bus_reset (dev,1 ); +if (reset_type &PCI_RESET_PM ) { + rc =pci_pm_reset (dev,1 ); +if (rc != -ENOTTY ) +return rc; + } + +if (reset_type &PCI_RESET_SLOT ) { + rc =pci_dev_reset_slot_function (dev,1 ); +if (rc != -ENOTTY ) +return rc; + } + +if (reset_type &PCI_RESET_BUS ) { + rc =pci_parent_bus_reset (dev,1 ); +if (rc != -ENOTTY ) +return rc; + } + +return rc; }/**
drivers/pci/pci.h +1 -1
@@ -33,7 +33,7 @@ enum pci_mmap_api {int pci_mmap_fits (struct pci_dev *pdev,int resno,struct vm_area_struct *vmai,enum pci_mmap_api mmap_api); -int pci_probe_reset_function (struct pci_dev *dev); +int pci_probe_reset_function (struct pci_dev *dev,u32 reset_type);int pci_bridge_secondary_bus_reset (struct pci_dev *dev);/**
drivers/pci/probe.c +1 -1
@@ -2332,7 +2332,7 @@ static void pci_init_capabilities(struct pci_dev *dev)pcie_report_downtraining (dev); -if (pci_probe_reset_function (dev) ==0 ) +if (pci_probe_reset_function (dev,PCI_RESET_ANY ) ==0 ) dev->reset_fn =1 ; }
Signature · 1 line
2.18.0
PATCH v4 6/6 PCI: Expose reset type to users of pci_reset_bus()
In reply to Sinan KayaLooking 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.
--- drivers/pci/pci.c | 18 +++++++++++++++--- drivers/vfio/pci/vfio_pci.c | 6 ++++-- include/linux/pci.h | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-)
drivers/pci/pci.c +15 -3
@@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)/** * pci_reset_bus - Try to reset a PCI bus * @pdev: top level PCI device to reset via slot/bus +* @reset_type: resets to try * * Same as above except return -EAGAIN if the bus cannot be locked */ -int pci_reset_bus (struct pci_dev *pdev) +int pci_reset_bus (struct pci_dev *pdev,u32 reset_type) { -return (!pci_probe_reset_slot (pdev->slot)) ? -__pci_reset_slot (pdev->slot) :__pci_reset_bus (pdev->bus); +if (reset_type &PCI_RESET_LINK ) { +return (!pci_probe_reset_slot (pdev->slot)) ? +__pci_reset_slot (pdev->slot) : +__pci_reset_bus (pdev->bus); + } + +if (reset_type &PCI_RESET_BUS ) +return __pci_reset_bus (pdev->bus); + +if (reset_type &PCI_RESET_SLOT ) +return __pci_reset_slot (pdev->slot); + +return -EINVAL ; }EXPORT_SYMBOL_GPL (pci_reset_bus);
drivers/vfio/pci/vfio_pci.c +4 -2
@@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data, &info, slot);if (!ret)/* User has access, do the reset */ - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); hot_reset_release:for (i--; i >=0 ; i--)
@@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) }if (needs_reset) - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); put_devs:for (i =0 ; i < devs.cur_index; i++) {
include/linux/pci.h +1 -1
@@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);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); +int pci_reset_bus (struct pci_dev *dev,u32 reset_type);void pci_reset_secondary_bus (struct pci_dev *dev);void pcibios_reset_secondary_bus (struct pci_dev *dev);void pci_update_resource (struct pci_dev *dev,int resno);
Signature · 1 line
2.18.0
Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()
Quoted 10 lines · On Wed, 19 Sep 2018 16:30:37 +0000 Sinan Kaya <[email protected]> wrote:
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 18 +++++++++++++++--- drivers/vfio/pci/vfio_pci.c | 6 ++++-- include/linux/pci.h | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-)
Quoted 70 lines
drivers/pci/pci.c +15
-3@@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)/** * pci_reset_bus - Try to reset a PCI bus * @pdev: top level PCI device to reset via slot/bus +* @reset_type: resets to try * * Same as above except return -EAGAIN if the bus cannot be locked */ -int pci_reset_bus (struct pci_dev *pdev) +int pci_reset_bus (struct pci_dev *pdev,u32 reset_type) { -return (!pci_probe_reset_slot (pdev->slot)) ? -__pci_reset_slot (pdev->slot) :__pci_reset_bus (pdev->bus); +if (reset_type &PCI_RESET_LINK ) { +return (!pci_probe_reset_slot (pdev->slot)) ? +__pci_reset_slot (pdev->slot) : +__pci_reset_bus (pdev->bus); + } + +if (reset_type &PCI_RESET_BUS ) +return __pci_reset_bus (pdev->bus); + +if (reset_type &PCI_RESET_SLOT ) +return __pci_reset_slot (pdev->slot); + +return -EINVAL ; }EXPORT_SYMBOL_GPL (pci_reset_bus);drivers/vfio/pci/vfio_pci.c +4
-2@@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data, &info, slot);if (!ret)/* User has access, do the reset */ - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); hot_reset_release:for (i--; i >=0 ; i--)@@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) }if (needs_reset) - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); put_devs:for (i =0 ; i < devs.cur_index; i++) {include/linux/pci.h +1
-1@@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);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); +int pci_reset_bus (struct pci_dev *dev,u32 reset_type);void pci_reset_secondary_bus (struct pci_dev *dev);void pcibios_reset_secondary_bus (struct pci_dev *dev);void pci_update_resource (struct pci_dev *dev,int resno);
Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()
In reply to Sinan KayaQuoted 29 lines · Sinan Kaya
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 18 +++++++++++++++--- drivers/vfio/pci/vfio_pci.c | 6 ++++-- include/linux/pci.h | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-)drivers/pci/pci.c +3
-3@@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)/** * pci_reset_bus - Try to reset a PCI bus * @pdev: top level PCI device to reset via slot/bus +* @reset_type: resets to try * * Same as above except return -EAGAIN if the bus cannot be locked */ -int pci_reset_bus (struct pci_dev *pdev) +int pci_reset_bus (struct pci_dev *pdev,u32 reset_type) { -return (!pci_probe_reset_slot (pdev->slot)) ? -__pci_reset_slot (pdev->slot) :__pci_reset_bus (pdev->bus); +if (reset_type &PCI_RESET_LINK ) {
drivers/pci/pci.c static int __pci_reset_bus(struct pci_bus *bus) · line 5251
Quoted 10 lines
drivers/pci/pci.c +10
-0+return (!pci_probe_reset_slot (pdev->slot)) ? +__pci_reset_slot (pdev->slot) : +__pci_reset_bus (pdev->bus); + } + +if (reset_type &PCI_RESET_BUS ) +return __pci_reset_bus (pdev->bus); + +if (reset_type &PCI_RESET_SLOT ) +return __pci_reset_slot (pdev->slot);
I don't understand this code. We have
#define PCI_RESET_LINK (PCI_RESET_SLOT | PCI_RESET_BUS)
so if either PCI_RESET_BUS or PCI_RESET_SLOT is set, we should take the first "if" above, which always returns, and the last two should never be reached. What am I missing?
Quoted 10 lines
drivers/pci/pci.c +2
-0+ +return -EINVAL ; }EXPORT_SYMBOL_GPL (pci_reset_bus);drivers/vfio/pci/vfio_pci.c +0
-0
drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 1019
Quoted 6 lines
drivers/vfio/pci/vfio_pci.c +2
-1@@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data, &info, slot);if (!ret)/* User has access, do the reset */ - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS );
This is the only place in the whole series where a caller uses something other than PCI_RESET_ANY. Inquiring minds want to know why. I'm sure it's the right thing to do, it's just that we'll need to know how to choose in future cases.
Quoted 29 lines
drivers/vfio/pci/vfio_pci.c +2
-1hot_reset_release:for (i--; i >=0 ; i--)@@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) }if (needs_reset) - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); put_devs:for (i =0 ; i < devs.cur_index; i++) {include/linux/pci.h +1
-1@@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);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); +int pci_reset_bus (struct pci_dev *dev,u32 reset_type);void pci_reset_secondary_bus (struct pci_dev *dev);void pcibios_reset_secondary_bus (struct pci_dev *dev);void pci_update_resource (struct pci_dev *dev,int resno);Signature · 1 line
2.18.0
Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()
Quoted 43 lines · Bjorn Helgaas
Quoted 29 lines · Sinan Kaya
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 18 +++++++++++++++--- drivers/vfio/pci/vfio_pci.c | 6 ++++-- include/linux/pci.h | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-)drivers/pci/pci.c +3
-3@@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)/** * pci_reset_bus - Try to reset a PCI bus * @pdev: top level PCI device to reset via slot/bus +* @reset_type: resets to try * * Same as above except return -EAGAIN if the bus cannot be locked */ -int pci_reset_bus (struct pci_dev *pdev) +int pci_reset_bus (struct pci_dev *pdev,u32 reset_type) { -return (!pci_probe_reset_slot (pdev->slot)) ? -__pci_reset_slot (pdev->slot) :__pci_reset_bus (pdev->bus); +if (reset_type &PCI_RESET_LINK ) {drivers/pci/pci.c static int __pci_reset_bus(struct pci_bus *bus) · line 5251
Quoted 10 lines
drivers/pci/pci.c +10
-0+return (!pci_probe_reset_slot (pdev->slot)) ? +__pci_reset_slot (pdev->slot) : +__pci_reset_bus (pdev->bus); + } + +if (reset_type &PCI_RESET_BUS ) +return __pci_reset_bus (pdev->bus); + +if (reset_type &PCI_RESET_SLOT ) +return __pci_reset_slot (pdev->slot);I don't understand this code. We have
Quoted 5 lines
#define PCI_RESET_LINK (PCI_RESET_SLOT | PCI_RESET_BUS)so if either
PCI_RESET_BUSorPCI_RESET_SLOTis set, we should take the first "if" above, which always returns, and the last two should never be reached. What am I missing?
Yup, this is broken. I need to follow up with another patchset.
I was trying to cover the case where user said,
"I need a link reset. I don't care whether it is a slot reset or bus reset as long as we achieve a reset."
That's when PCI_RESET_LINK is used. We expect most drivers to PCI_RESET_LINK if they are interested in a link reset and doesn't need to know about the hotplug capability of a particular slot.
Quoted 6 lines
Quoted 6 lines
drivers/pci/pci.c +2
-0+ +return -EINVAL ; }EXPORT_SYMBOL_GPL (pci_reset_bus);
Quoted 16 lines
drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 1019
Quoted 11 lines
drivers/vfio/pci/vfio_pci.c +2
-1@@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data, &info, slot);if (!ret)/* User has access, do the reset */ - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS );This is the only place in the whole series where a caller uses something other than
PCI_RESET_ANY. Inquiring minds want to know why. I'm sure it's the right thing to do, it's just that we'll need to know how to choose in future cases.
Use PCI_RESET_ANY for the existing behavior where you want one type of reset and don't care if it is PM/function/slot/bus/specific.
Use PCI_RESET_LINK when you need link to retrain.
Use PCI_RESET_SLOT when you know that this system is hotplug capable by calling probe functions.
Use PCI_RESET_BUS when you know that this system is not hotplug capable and this endpoint will never be used in such a system.
I can capture this into the commit message.
Quoted 31 lines
Quoted 30 lines
drivers/vfio/pci/vfio_pci.c +2
-1hot_reset_release:for (i--; i >=0 ; i--)@@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) }if (needs_reset) - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); put_devs:for (i =0 ; i < devs.cur_index; i++) {include/linux/pci.h +1
-1@@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);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); +int pci_reset_bus (struct pci_dev *dev,u32 reset_type);void pci_reset_secondary_bus (struct pci_dev *dev);void pcibios_reset_secondary_bus (struct pci_dev *dev);void pci_update_resource (struct pci_dev *dev,int resno);Signature · 1 line
2.18.0
Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()
Quoted 83 lines · Sinan Kaya
Quoted 43 lines · Bjorn Helgaas
Quoted 29 lines · Sinan Kaya
Looking to have more control between the users of the API vs. what the API can do internally. The new
reset_typetells the PCI core about the bounds of the request.Signed-off-by Sinan Kaya <[email protected]>--- drivers/pci/pci.c | 18 +++++++++++++++--- drivers/vfio/pci/vfio_pci.c | 6 ++++-- include/linux/pci.h | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-)drivers/pci/pci.c +3
-3@@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)/** * pci_reset_bus - Try to reset a PCI bus * @pdev: top level PCI device to reset via slot/bus +* @reset_type: resets to try * * Same as above except return -EAGAIN if the bus cannot be locked */ -int pci_reset_bus (struct pci_dev *pdev) +int pci_reset_bus (struct pci_dev *pdev,u32 reset_type) { -return (!pci_probe_reset_slot (pdev->slot)) ? -__pci_reset_slot (pdev->slot) :__pci_reset_bus (pdev->bus); +if (reset_type &PCI_RESET_LINK ) {drivers/pci/pci.c static int __pci_reset_bus(struct pci_bus *bus) · line 5251
Quoted 10 lines
drivers/pci/pci.c +10
-0+return (!pci_probe_reset_slot (pdev->slot)) ? +__pci_reset_slot (pdev->slot) : +__pci_reset_bus (pdev->bus); + } + +if (reset_type &PCI_RESET_BUS ) +return __pci_reset_bus (pdev->bus); + +if (reset_type &PCI_RESET_SLOT ) +return __pci_reset_slot (pdev->slot);I don't understand this code. We have
Quoted 5 lines
#define PCI_RESET_LINK (PCI_RESET_SLOT | PCI_RESET_BUS)so if either
PCI_RESET_BUSorPCI_RESET_SLOTis set, we should take the first "if" above, which always returns, and the last two should never be reached. What am I missing?Yup, this is broken. I need to follow up with another patchset.
I was trying to cover the case where user said,
"I need a link reset. I don't care whether it is a slot reset or bus reset as long as we achieve a reset."
That's when
PCI_RESET_LINKis used. We expect most drivers toPCI_RESET_LINKif they are interested in a link reset and doesn't need to know about the hotplug capability of a particular slot.Quoted 13 lines
Quoted 13 lines
drivers/pci/pci.c +2
-0+ +return -EINVAL ; }EXPORT_SYMBOL_GPL (pci_reset_bus);drivers/vfio/pci/vfio_pci.c +0
-0@@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data, &info, slot);if (!ret)/* User has access, do the reset */ Quoted 8 lines
drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 1019
Quoted 3 lines
drivers/vfio/pci/vfio_pci.c +2
-1- ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS );This is the only place in the whole series where a caller uses something other than
PCI_RESET_ANY. Inquiring minds want to know why. I'm sure it's the right thing to do, it's just that we'll need to know how to choose in future cases.
Quoted 4 lines
Use
PCI_RESET_ANYfor the existing behavior where you want one type of reset and don't care if it is PM/function/slot/bus/specific.Use
PCI_RESET_LINKwhen you need link to retrain.
There are no callers using this. Is this intended specifically for the case of "the hardware wasn't smart enough to train at the correct speed, so we fiddled things and want to retrain"?
Maybe it doesn't need to be exposed in include/linux/pci.h and could be defined internally in drivers/pci/pci.c if it's needed there?
Quoted 2 lines
Use
PCI_RESET_SLOTwhen you know that this system is hotplug capable by calling probe functions.
I raise my eyebrows at this because (a) a driver generally can't tell whether hotplug is supported and (b) even if the driver does know, what is the benefit to the driver to specify this? What probe functions does this refer to? If I'm a driver writer, I really can't tell what I'm supposed to do with this guidance. If I'm supposed to call a probe function, what is it, when should I call it, and what should I do with the result? Am I supposed to know whether hotplug is supported? Why would I care?
I'm sure you have good answers for all these questions; I just don't know what they are :)
Quoted 2 lines
Use
PCI_RESET_BUSwhen you know that this system is not hotplug capable and this endpoint will never be used in such a system.
How can a driver know this? And what's the benefit of being specific?
Quoted 1 line
I can capture this into the commit message.
I think it needs to be more accessible, e.g., comments near the constants and/or the function declaration. We shouldn't expect users of the interface to dig through the changelogs for it.
Quoted 28 lines
Quoted 27 lines
Quoted 26 lines
drivers/vfio/pci/vfio_pci.c +2
-1hot_reset_release:for (i--; i >=0 ; i--)@@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) }if (needs_reset) - ret =pci_reset_bus (vdev->pdev); + ret =pci_reset_bus (vdev->pdev, + slot ?PCI_RESET_SLOT :PCI_RESET_BUS ); put_devs:for (i =0 ; i < devs.cur_index; i++) {include/linux/pci.h +1
-1@@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);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); +int pci_reset_bus (struct pci_dev *dev,u32 reset_type);void pci_reset_secondary_bus (struct pci_dev *dev);void pcibios_reset_secondary_bus (struct pci_dev *dev);void pci_update_resource (struct pci_dev *dev,int resno);Signature · 1 line
2.18.0
Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()
Quoted 5 lines · Bjorn Helgaas
Quoted 1 line
Use
PCI_RESET_LINKwhen you need link to retrain.There are no callers using this. Is this intended specifically for the case of "the hardware wasn't smart enough to train at the correct speed, so we fiddled things and want to retrain"?
Yup, I'll go ahead and change the hfi1 driver to use PCI_RESET_LINK as originally planned so that there is an actual user.
These constants are intended to be used by the user of pci_reset APIs. I think it makes sense to keep them there as well. I see your point that PCI_RESET_LINK was not used. We should also make the hfi1 follow the rules.
Quoted 3 lines
Maybe it doesn't need to be exposed in
include/linux/pci.hand could be defined internally indrivers/pci/pci.cif it's needed there?
see above.
Quoted 10 lines
Quoted 2 lines
Use
PCI_RESET_SLOTwhen you know that this system is hotplug capable by calling probe functions.I raise my eyebrows at this because (a) a driver generally can't tell whether hotplug is supported and (b) even if the driver does know, what is the benefit to the driver to specify this? What probe functions does this refer to? If I'm a driver writer, I really can't tell what I'm supposed to do with this guidance. If I'm supposed to call a probe function, what is it, when should I call it, and what should I do with the result? Am I supposed to know whether hotplug is supported? Why would I care?
That was the whole reason why I hid the secondary bus reset API from the user and folded into pci_reset_bus() so that PCI core can deal with hotplug internally and can go to hotplug reset or bus reset internally.
User just calls pci_reset_bus().
I fully agree with your assessment but there are also exceptions like VFIO where the code finds out which particular devices are part of a slot hierarchy and for each one it checks that VFIO ownership has been claimed. (Alex, please correct me if I'm not getting this right. I just read 200 lines of code in VFIO)
/* Can we do a slot or bus reset or neither? */ if (!pci_probe_reset_slot(vdev->pdev->slot)) slot = true; else if (pci_probe_reset_bus(vdev->pdev->bus)) return -ENODEV; ret = vfio_pci_for_each_slot_or_bus(vdev->pdev, vfio_pci_fill_devs, &fill, slot);
Quoted 7 lines
I'm sure you have good answers for all these questions; I just don't know what they are:)
Quoted 2 lines
Use
PCI_RESET_BUSwhen you know that this system is not hotplug capable and this endpoint will never be used in such a system.How can a driver know this? And what's the benefit of being specific?
There is really no benefit but there are drivers making assumptions about the type of platform they want to run like supporting single segment only etc.
PCI_RESET_BUS gives you direct access to secondary bus reset. If you are calling this directly, you are responsible for saving and restoring state like the hfi1 driver and need to ensure that this card will never work on a hotplug capable system. If not, you are ...ed.
That's why, I'm suggesting that most users will use PCI_RESET_LINK so that code works on all platforms.
Quoted 6 lines
Quoted 2 lines
I can capture this into the commit message.
I think it needs to be more accessible, e.g., comments near the constants and/or the function declaration. We shouldn't expect users of the interface to dig through the changelogs for it.
I can work on this once we agree if this is the way to go. I was responding to Alex's request to have a contact between the PCI core and the drivers because there are some driver that are more intelligent about the PCI tree than a simple endpoint driver.
Quoted 14 lines · On Wed, 19 Sep 2018 16:30:31 +0000 Sinan Kaya <[email protected]> wrote:
We want more control over what kind of reset PCI core can apply. Have the application explicitly requested allowed reset types.
differences from v3:
- set default return value to-EINVALin__pci_reset_function_locked()Sinan Kaya (6):
PCI: Exposereset_typeto users of__pci_reset_function_locked()
PCI: Exposereset_typeto users ofpci_reset_function()
PCI: Exposereset_typeto users ofpci_reset_function_locked()
PCI: Expose reset type to users ofpci_try_reset_function()
PCI: Expose reset type to users ofpci_probe_reset_function()
PCI: Expose reset type to users ofpci_reset_bus()
Quoted 15 lines
drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- drivers/misc/genwqe/card_base.c | 2 +- .../net/ethernet/cavium/liquidio/lio_main.c | 2 +- .../net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 2 +- drivers/net/ethernet/sfc/mcdi.c | 2 +- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/pci.c | 134 ++++++++++++------ drivers/pci/pci.h | 2 +- drivers/pci/probe.c | 2 +- drivers/vfio/pci/vfio_pci.c | 13 +- drivers/vfio/pci/vfio_pci_config.c | 4 +- drivers/xen/xen-pciback/pci_stub.c | 6 +- include/linux/pci.h | 21 ++- 13 files changed, 128 insertions(+), 66 deletions(-)
Ack'd 4 & 6, for the remainder,