| Message ID | 20181011045008.32212-8-okaya@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v5,01/11] PCI: Expose reset_type to users of __pci_reset_function_locked() | expand |
On Thu, 11 Oct 2018 04:50:00 +0000 Sinan Kaya <okaya@kernel.org> wrote: > The difference between pci_reset_function_locked() and > __pci_reset_function_locked() is the saving and restoring of the registers. > Unify these API by adding saverestore argument that caller passes. > > Signed-off-by: Sinan Kaya <okaya@kernel.org> > --- > drivers/infiniband/hw/hfi1/pcie.c | 2 +- > drivers/net/ethernet/cavium/liquidio/lio_main.c | 2 +- > drivers/pci/pci.c | 10 +++++++--- > drivers/pci/pci.h | 1 + > drivers/xen/xen-pciback/pci_stub.c | 6 +++--- > include/linux/pci.h | 4 ++-- > 6 files changed, 15 insertions(+), 10 deletions(-) TBH, I'm not a fan of this patch or the remainder in this series. Having a function prefixed with underscored or specifically indicate locked tells callers that these are special cases and should be understood before using. Adding bool parameters to the common functions ensures that every caller now needs to understand those special cases and potentially get them wrong. Also, a string of random bool options to a function means that any time we want to use it we need to go reexamine the function definition. It's not intuitive to use or review. Thanks, Alex
On 10/11/2018 12:02 PM, Alex Williamson wrote: >> The difference between pci_reset_function_locked() and >> __pci_reset_function_locked() is the saving and restoring of the registers. >> Unify these API by adding saverestore argument that caller passes. >> >> Signed-off-by: Sinan Kaya<okaya@kernel.org> >> --- >> drivers/infiniband/hw/hfi1/pcie.c | 2 +- >> drivers/net/ethernet/cavium/liquidio/lio_main.c | 2 +- >> drivers/pci/pci.c | 10 +++++++--- >> drivers/pci/pci.h | 1 + >> drivers/xen/xen-pciback/pci_stub.c | 6 +++--- >> include/linux/pci.h | 4 ++-- >> 6 files changed, 15 insertions(+), 10 deletions(-) > TBH, I'm not a fan of this patch or the remainder in this series. > Having a function prefixed with underscored or specifically indicate > locked tells callers that these are special cases and should be > understood before using. Adding bool parameters to the common > functions ensures that every caller now needs to understand those > special cases and potentially get them wrong. Also, a string of random > bool options to a function means that any time we want to use it we > need to go reexamine the function definition. It's not intuitive to > use or review. Thanks, Thanks for the feedback. I wanted to get it out to gather feedback. I think it is quite straightforward for people familiar with the existing API to know what they are dealing with. I didn't know the difference between __pci_reset_function_locked() and pci_reset_function_locked() for a long time. This was an attempt to minimize reset function flavors and ask explicitly what user wants rather than having them use the wrong function and suffer the consequence with random failures.
On Thu, Oct 11, 2018 at 04:50:00AM +0000, Sinan Kaya wrote: > The difference between pci_reset_function_locked() and > __pci_reset_function_locked() is the saving and restoring of the registers. > Unify these API by adding saverestore argument that caller passes. Adding random boolean arguments doesn't really help the API. Either make this another flag for reset_type or if there is a clear benefit add an additional flags parameter with well described flags.
On 10/12/2018 5:18 AM, Christoph Hellwig wrote: > On Thu, Oct 11, 2018 at 04:50:00AM +0000, Sinan Kaya wrote: >> The difference between pci_reset_function_locked() and >> __pci_reset_function_locked() is the saving and restoring of the registers. >> Unify these API by adding saverestore argument that caller passes. > > Adding random boolean arguments doesn't really help the API. Either > make this another flag for reset_type or if there is a clear benefit > add an additional flags parameter with well described flags. > Good idea. This is the kind of feedback I was looking for. Here is what I collected so far. Alex doesn't like the API changes as it should be obvious that there are differences between pci_reset_function_locked() and __pci_reset_function_locked(). Sinan thinks that the difference is not that obvious as some drivers implemented their own save/restore mechanism using __pci_reset_function_locked(). Sinan would rather reduce the reset API flavors and give a unified interface where user specifies what they are interested in. Christoph asked for additional flags to be rolled into the reset_type argument. Sinan thinks that this is a good idea. Anybody else?
diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c index 38f96192e5f0..73eced9c5523 100644 --- a/drivers/infiniband/hw/hfi1/pcie.c +++ b/drivers/infiniband/hw/hfi1/pcie.c @@ -897,7 +897,7 @@ static int trigger_sbr(struct hfi1_devdata *dd) * to be implemented to have cleaner interface but this fixes the * current brokenness */ - return __pci_reset_function_locked(dev, PCI_RESET_LINK); + return pci_reset_function_locked(dev, PCI_RESET_LINK, false); } /* diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c index 0ff76722734d..7e001382ad93 100644 --- a/drivers/net/ethernet/cavium/liquidio/lio_main.c +++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c @@ -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, PCI_RESET_ANY); + rc = pci_reset_function_locked(oct->pci_dev, PCI_RESET_ANY, false); if (rc != 0) dev_err(&oct->pci_dev->dev, "Error %d resetting PCI function %d\n", diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index a3a1c2a5e0cf..97328dc8e476 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4835,6 +4835,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function); * pci_reset_function_locked - quiesce and reset a PCI device function * @dev: PCI device to reset * @reset_type: reset type to apply + * @saverestore: Flag indicating whether device context should be saved. * * Some devices allow an individual function to be reset without affecting * other functions in the same device. The PCI device must be responsive @@ -4849,18 +4850,21 @@ 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, u32 reset_type) +int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type, + bool saverestore) { int rc; if (!dev->reset_fn) return -ENOTTY; - pci_dev_save_and_disable(dev); + if (saverestore) + pci_dev_save_and_disable(dev); rc = __pci_reset_function_locked(dev, reset_type); - pci_dev_restore(dev); + if (saverestore) + pci_dev_restore(dev); return rc; } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 0444bfa51b52..fbfb44fb32b7 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -35,6 +35,7 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai, int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type); int pci_bridge_secondary_bus_reset(struct pci_dev *dev); +int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type); /** * struct pci_platform_pm_ops - Firmware PM callbacks diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index 6dfb805bcb19..b68e811ab27f 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -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_ANY); + pci_reset_function_locked(dev, PCI_RESET_ANY, false); 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_ANY); + pci_reset_function_locked(dev, PCI_RESET_ANY, false); 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_ANY); + pci_reset_function_locked(dev, PCI_RESET_ANY, false); pci_restore_state(dev); } /* Now disable the device (this also ensures some private device diff --git a/include/linux/pci.h b/include/linux/pci.h index d4acdc400ef2..84d08a9a01e3 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1165,9 +1165,9 @@ 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, 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_reset_function_locked(struct pci_dev *dev, u32 reset_type, + bool saverestore); 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);
The difference between pci_reset_function_locked() and __pci_reset_function_locked() is the saving and restoring of the registers. Unify these API by adding saverestore argument that caller passes. Signed-off-by: Sinan Kaya <okaya@kernel.org> --- drivers/infiniband/hw/hfi1/pcie.c | 2 +- drivers/net/ethernet/cavium/liquidio/lio_main.c | 2 +- drivers/pci/pci.c | 10 +++++++--- drivers/pci/pci.h | 1 + drivers/xen/xen-pciback/pci_stub.c | 6 +++--- include/linux/pci.h | 4 ++-- 6 files changed, 15 insertions(+), 10 deletions(-)