From: Kuppuswamy Sathyanarayanan <[email protected]>
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.
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
Documentation/PCI/pcieaer-howto.rst | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/Documentation/PCI/pcieaer-howto.rst b/Documentation/PCI/pcieaer-howto.rst
index 18bdefaafd1a..0f3e5880efb8 100644
--- a/Documentation/PCI/pcieaer-howto.rst
+++ b/Documentation/PCI/pcieaer-howto.rst
@@ -147,7 +147,7 @@ section 3.3.
Provide callbacks
-----------------
-callback reset_link to reset pci express link
+callback reset_cb to reset pci express link
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This callback is used to reset the pci express physical link when a
@@ -156,11 +156,8 @@ default reset_link function, but different upstream ports might
have different specifications to reset pci express link, so all
upstream ports should provide their own reset_link functions.
-In struct pcie_port_service_driver, a new pointer, reset_link, is
-added.
-::
-
- pci_ers_result_t (*reset_link) (struct pci_dev *dev);
+In pcie_do_recovery function, reset_cb function pointer can be used
+to pass the port specific reset_link callback.
Section 3.2.2.2 provides more detailed info on when to call
reset_link.
@@ -212,13 +209,13 @@ error_detected(dev, pci_channel_io_frozen) to all drivers within
a hierarchy in question. Then, performing link reset at upstream is
necessary. As different kinds of devices might use different approaches
to reset link, AER port service driver is required to provide the
-function to reset link. Firstly, kernel looks for if the upstream
-component has an aer driver. If it has, kernel uses the reset_link
-callback of the aer driver. If the upstream component has no aer driver
-and the port is downstream port, we will perform a hot reset as the
-default by setting the Secondary Bus Reset bit of the Bridge Control
-register associated with the downstream port. As for upstream ports,
-they should provide their own aer service drivers with reset_link
+function to reset link via reset_cb parameter of pcie_do_recovery()
+function call. If reset_cb is not NULL, recovery function will use it
+to reset the link. If there is no reset_cb callback provided and
+the port is downstream port, we will perform a hot reset as the default
+by setting the Secondary Bus Reset bit of the Bridge Control register
+associated with the downstream port. As for upstream ports,
+they should provide their own reset_link function via reset_cb callback
function. If error_detected returns PCI_ERS_RESULT_CAN_RECOVER and
reset_link returns PCI_ERS_RESULT_RECOVERED, the error handling goes
to mmio_enabled.
@@ -262,7 +259,7 @@ A:
Q:
What happens if an upstream port service driver does not provide
- callback reset_link?
+ callback reset_cb?
A:
Fatal error recovery will fail if the errors are reported by the
--
2.25.1
On Tue, Mar 03, 2020 at 06:36:29PM -0800, [email protected] wrote:
> From: Kuppuswamy Sathyanarayanan <[email protected]>
>
> 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.
On 3/17/20 7:42 AM, Christoph Hellwig wrote:
> On Tue, Mar 03, 2020 at 06:36:29PM -0800, [email protected] wrote:
>> From: Kuppuswamy Sathyanarayanan <[email protected]>
>>
>
> 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
>
On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > From: Kuppuswamy Sathyanarayanan <[email protected]>
> > >
> >
> > 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?
On Tue, Mar 17, 2020 at 10:09 AM Christoph Hellwig <[email protected]> wrote:
>
> On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > From: Kuppuswamy Sathyanarayanan <[email protected]>
> > > >
> > >
> > > 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
On Tue, Mar 17, 2020 at 11:03:36AM -0500, Bjorn Helgaas wrote:
> On Tue, Mar 17, 2020 at 10:09 AM Christoph Hellwig <[email protected]> wrote:
> >
> > On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <[email protected]>
> > > > >
> > > >
> > > > 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;
...
On Tue, Mar 17, 2020 at 10:06:54AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 17, 2020 at 11:03:36AM -0500, Bjorn Helgaas wrote:
> > On Tue, Mar 17, 2020 at 10:09 AM Christoph Hellwig <[email protected]> wrote:
> > > On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > > > From: Kuppuswamy Sathyanarayanan <[email protected]>
> > > > >
> > > > > 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.
Merged, thanks.
> PCI/DPC: Move DPC data into struct pci_dev:
>
> - dpc_rp_extensions probable should be a "bool : 1"
I actually had not seen "bool : 1" used, but you're right, there are
several. There aren't any in drivers/pci, though, so I'm inclined to
stay consistent with "unsigned int : 1" unless there's an advantage,
and then I'd probably convert all of drivers/pci over.
My rule of thumb has been [1], where Linus suggests "unsigned int
percpu:1", but maybe that should be updated.
> 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.
Agreed, thanks, I removed that dead code.
> 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.
I didn't do this because it sounds like it might be a separate patch.
But maybe Sathy can do this in the next round?
> 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;
> ...
Nice, thanks! I made this change, too.
Thanks a lot for reviewing this!
Bjorn
[1] https://lore.kernel.org/linux-arm-kernel/CA+55aFxnePDimkVKVtv3gNmRGcwc8KQ5mHYvUxY8sAQg6yvVYg@mail.gmail.com/