On 2021/3/9 23:50, Bjorn Helgaas wrote:

[+cc Alex, Krzysztof]

On Wed, Nov 11, 2020 at 06:22:03PM +0800, Yicong Yang wrote:

...

Previosly we use pci_probe_reset_function() to probe whehter a function can be reset and use __pci_reset_function_locked() to perform a function reset. These two functions have lots of common lines.

s/Previosly/Previously/ s/we use/we used/ s/whehter/whether/

will fix.

I'm not a big fan of dual-purpose functions that decide whether to probe or to reset based on a parameter. Those two things just don't seem semantically compatible. But I do see your point about common lines, and the underlying functions (pci_dev_specific_reset(), pci_af_flr(), etc) already have that structure.

i noticed this when i tried to fix another issue with FLR[1]. as you said, the underlying functions use the probe flag to indicate whether it is a probe operation or not and i think it make sense to reduce the redundency in the same way.

Let me think about this some more.

sure, of course. It's also very kind to have any comments on the previous issue we met: [1] https://lore.kernel.org/linux-pci/1605090088-13960-1-git-send-email-yangyico... Thanks, Yicong

...

Factor the two functions and reduce the redundancy.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/pci/pci.c | 61 ++++++++++++++++------------------------------------- drivers/pci/pci.h | 2 +- drivers/pci/probe.c | 2 +- 3 files changed, 20 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e39c549..e3e5f0f 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5006,9 +5006,11 @@ static void pci_dev_restore(struct pci_dev *dev) }

/** - * __pci_reset_function_locked - reset a PCI device function while holding - * the @dev mutex lock. + * pci_probe_reset_function - check whether the device can be safely reset + * or reset a PCI device function while holding + * the @dev mutex lock. * @dev: PCI device to reset + * @probe: Probe or not whether the device can be reset. * * Some devices allow an individual function to be reset without affecting * other functions in the same device. The PCI device must be responsive @@ -5022,10 +5024,10 @@ static void pci_dev_restore(struct pci_dev *dev) * device including MSI, bus mastering, BARs, decoding IO and memory spaces, * etc. * - * Returns 0 if the device function was successfully reset or negative if the - * device doesn't support resetting a single function. + * Returns 0 if the device function can be reset or was successfully reset. + * negative if the device doesn't support resetting a single function. */ -int __pci_reset_function_locked(struct pci_dev *dev) +int pci_probe_reset_function(struct pci_dev *dev, int probe) { int rc;

@@ -5039,61 +5041,34 @@ int __pci_reset_function_locked(struct pci_dev *dev) * other error, we're also finished: this indicates that further * reset mechanisms might be broken on the device. */ - rc = pci_dev_specific_reset(dev, 0); + rc = pci_dev_specific_reset(dev, probe); if (rc != -ENOTTY) return rc; if (pcie_has_flr(dev)) { + if (probe) + return 0; rc = pcie_flr(dev); if (rc != -ENOTTY) return rc; } - rc = pci_af_flr(dev, 0); + rc = pci_af_flr(dev, probe); if (rc != -ENOTTY) return rc; - rc = pci_pm_reset(dev, 0); + rc = pci_pm_reset(dev, probe); if (rc != -ENOTTY) return rc; - rc = pci_dev_reset_slot_function(dev, 0); + rc = pci_dev_reset_slot_function(dev, probe); if (rc != -ENOTTY) return rc; - return pci_parent_bus_reset(dev, 0); + + return pci_parent_bus_reset(dev, probe); } -EXPORT_SYMBOL_GPL(__pci_reset_function_locked);

-/** - * pci_probe_reset_function - check whether the device can be safely reset - * @dev: PCI device to reset - * - * Some devices allow an individual function to be reset without affecting - * other functions in the same device. The PCI device must be responsive - * to PCI config space in order to use this function. - * - * 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_reset_function_locked(struct pci_dev *dev) { - int rc; - - 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; - - return pci_parent_bus_reset(dev, 1); + return pci_probe_reset_function(dev, 0); } +EXPORT_SYMBOL_GPL(__pci_reset_function_locked);

/** * pci_reset_function - quiesce and reset a PCI device function diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index fa12f7c..73740dd 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -39,7 +39,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, int probe); int pci_bridge_secondary_bus_reset(struct pci_dev *dev); int pci_bus_error_reset(struct pci_dev *dev);

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 03d3712..793cc8a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2403,7 +2403,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, 1) == 0) dev->reset_fn = 1; }

-- 2.8.1

.