diff mbox series

[v17,06/12] Documentation: PCI: Remove reset_link references

Message ID a46938d227f6a11c010943800450a10aac39b7d3.1583286655.git.sathyanarayanan.kuppuswamy@linux.intel.com
State New
Headers show
Series Add Error Disconnect Recover (EDR) support | expand

Commit Message

Kuppuswamy Sathyanarayanan March 4, 2020, 2:36 a.m. UTC 
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

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 <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 Documentation/PCI/pcieaer-howto.rst | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Comments

Christoph Hellwig March 17, 2020, 2:42 p.m. UTC | #1 
On Tue, Mar 03, 2020 at 06:36:29PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> 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 March 17, 2020, 3:05 p.m. UTC | #2 
On 3/17/20 7:42 AM, Christoph Hellwig wrote:
> On Tue, Mar 03, 2020 at 06:36:29PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
> 
> 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 March 17, 2020, 3:07 p.m. UTC | #3 
On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > 
> > 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 March 17, 2020, 4:03 p.m. UTC | #4 
On Tue, Mar 17, 2020 at 10:09 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > >
> > >
> > > 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 March 17, 2020, 5:06 p.m. UTC | #5 
On Tue, Mar 17, 2020 at 11:03:36AM -0500, Bjorn Helgaas wrote:
> On Tue, Mar 17, 2020 at 10:09 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > >
> > > >
> > > > 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;
	...
Bjorn Helgaas March 19, 2020, 10:52 p.m. UTC | #6 
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 <hch@infradead.org> wrote:
> > > On Tue, Mar 17, 2020 at 08:05:50AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > >
> > > > > 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/
diff mbox series

Patch

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