[PATCH v17 03/12] PCI/ERR: Remove service dependency in pcie_do_recovery()

[Christoph Hellwig]

-static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
+static pci_ers_result_t reset_link(struct pci_dev *dev,
+ pci_ers_result_t (*reset_cb)(struct pci_dev *pdev))
{
pci_ers_result_t status;
- struct pcie_port_service_driver *driver = NULL;
- driver = pcie_port_find_service(dev, service);
- if (driver && driver->reset_link) {
- status = driver->reset_link(dev);
+ if (reset_cb) {
+ status = reset_cb(dev);

As far as I can tell reset_cb is never NULL. So all the code below
is dead, and the remainder of reset_link is so trivial that it can
be inlined into the only caller.

[Christoph Hellwig]

reset_link member in struct pcie_port_service_driver was
mainly added to let pcie_do_recovery() trigger the driver
specific reset_link() on PCIe fatal errors. But after
modifying the pcie_do_recovery() function to accept reset_link
callback as function parameter, we no longer have need to use
or set reset_link in struct pcie_port_service_driver. So remove
it.

This should be folded into
"PCI/ERR: Remove service dependency in pcie_do_recovery()"

[Kuppuswamy, Sathyanarayanan]

Hi,
it.

Post by Christoph Hellwig
This should be folded into
"PCI/ERR: Remove service dependency in pcie_do_recovery()"

I think Bjorn already folded them together. Please check.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

[Christoph Hellwig]

After pcie_do_recovery() refactor, instead of reset_link
member in struct pcie_port_service_driver, we use reset_cb
callback parameter in pcie_do_recovery() function to pass
the service driver specific reset_link function. So modify
the Documentation to reflect the latest changes.

This should be folded into the patch removing the method.

[Kuppuswamy, Sathyanarayanan]

Post by Christoph Hellwig
This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

[Christoph Hellwig]

Post by Kuppuswamy, Sathyanarayanan

Post by Christoph Hellwig
This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

I can't find that series anywhere on the list. What did I miss?

[Bjorn Helgaas]

Post by Christoph Hellwig

Post by Kuppuswamy, Sathyanarayanan

Post by Christoph Hellwig
This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

I can't find that series anywhere on the list. What did I miss?

We've still been discussing other issues (access to AER registers,
synchronization between EDR and hotplug, etc) in other parts of this
thread. The git branch Sathy pointed to above is my local branch.
I'll send it to the list before putting it into -next, but I wanted to
make progress on some of these other issues first.

Bjorn

[Christoph Hellwig]

Post by Bjorn Helgaas

Post by Christoph Hellwig

Post by Kuppuswamy, Sathyanarayanan

Post by Christoph Hellwig
This should be folded into the patch removing the method.

This is also folded in the mentioned patch.
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=review/edr&id=7a18dc6506f108db3dc40f5cd779bc15270c4183

I can't find that series anywhere on the list. What did I miss?

We've still been discussing other issues (access to AER registers,
synchronization between EDR and hotplug, etc) in other parts of this
thread. The git branch Sathy pointed to above is my local branch.
I'll send it to the list before putting it into -next, but I wanted to
make progress on some of these other issues first.

A few nitpicks:

PCI/ERR: Update error status after reset_link():

- there are two "if (state == pci_channel_io_frozen)"
right after each other now, merging them would make the code a little
easier to read.

PCI/DPC: Move DPC data into struct pci_dev:

- dpc_rp_extensions probable should be a "bool : 1"

PCI/ERR: Remove service dependency in pcie_do_recovery():

- as mentioned to Kuppuswamy the reset_cb is never NULL, and thus
a lot of dead code in reset_link can be removed. Also reset_link
should be merged into pcie_do_recovery. That would also enable
to call the argument reset_link, which might be a bit more
descriptive than reset_cb.

PCI/DPC: Cache DPC capabilities in pci_init_capabilities():

- I think the pci_dpc_init could be cleaned up a bit to:

...
pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
if (!(cap & PCI_EXP_DPC_CAP_RP_EXT))
return;
pdev->dpc_rp_extensions = true;
pdev->dpc_rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
...

[Christoph Hellwig]

Nothing is actually exported here (fortunately!).