diff mbox series

[v3,07/10] PCI: Add 'rcec' field to pci_dev for associated RCiEPs

Message ID 20200812164659.1118946-8-sean.v.kelley@intel.com
State New
Headers show
Series Add RCEC handling to PCI/AER | expand

Commit Message

Sean V Kelley Aug. 12, 2020, 4:46 p.m. UTC
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

When attempting error recovery for an RCiEP associated with an RCEC device,
there needs to be a way to update the Root Error Status, the Uncorrectable
Error Status and the Uncorrectable Error Severity of the parent RCEC.
So add the 'rcec' field to the pci_dev structure and provide a hook for the
Root Port Driver to associate RCiEPs with their respective parent RCEC.

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/aer.c         |  9 +++++----
 drivers/pci/pcie/err.c         | 10 ++++++++++
 drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
 include/linux/pci.h            |  1 +
 4 files changed, 31 insertions(+), 4 deletions(-)

Comments

Bjorn Helgaas Sept. 2, 2020, 4:35 p.m. UTC | #1
On Wed, Aug 12, 2020 at 09:46:56AM -0700, Sean V Kelley wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> When attempting error recovery for an RCiEP associated with an RCEC device,
> there needs to be a way to update the Root Error Status, the Uncorrectable
> Error Status and the Uncorrectable Error Severity of the parent RCEC.
> So add the 'rcec' field to the pci_dev structure and provide a hook for the
> Root Port Driver to associate RCiEPs with their respective parent RCEC.

> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -202,6 +202,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  		pci_walk_dev_affected(dev, report_frozen_detected, &status);
>  		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>  			status = flr_on_rciep(dev);
> +			/*
> +			 * The callback only clears the Root Error Status
> +			 * of the RCEC (see aer.c).
> +			 */
> +			if (dev->rcec)
> +				reset_link(dev->rcec);
>  			if (status != PCI_ERS_RESULT_RECOVERED) {
>  				pci_warn(dev, "function level reset failed\n");
>  				goto failed;
> @@ -245,7 +251,11 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
>  		pci_aer_clear_device_status(dev);
>  		pci_aer_clear_nonfatal_status(dev);
> +	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END && dev->rcec) {
> +		pci_aer_clear_device_status(dev->rcec);
> +		pci_aer_clear_nonfatal_status(dev->rcec);

Conceptually, I'm not sure why we need the dev->rcec link.  The error
was *reported* via the RCEC, so don't we know the RCEC up front,
before we even identify the RCiEP?  Can't we just remember that and
dispense with dev->rcec?

I'm also concerned that if we fail to identify the RCiEP (i.e., we
don't have a valid "dev" to use dev->rcec), we will fail to clear the
error status bits.  I think it's possible that the RCEC will report an
error, but the RCiEP that generated the error message is not
responding so we can't find it.

>  	}
> +
>  	pci_info(dev, "device recovery successful\n");
>  	return status;
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index d5b109499b10..a64e88b7166b 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -90,6 +90,18 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  #define PCIE_PORTDRV_PM_OPS	NULL
>  #endif /* !PM */
>  
> +static int pcie_hook_rcec(struct pci_dev *pdev, void *data)
> +{
> +	struct pci_dev *rcec = (struct pci_dev *)data;
> +
> +	pdev->rcec = rcec;
> +	pci_dbg(rcec, "RCiEP(under an RCEC) %04x:%02x:%02x.%d\n",
> +		pci_domain_nr(pdev->bus), pdev->bus->number,
> +		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));

If we do need dev->rcec, this should use pci_name() for the second
device instead of formatting the name manually.  I think I would
connect this with the RCiEP instead of the RCEC, e.g.,

  pci_dbg(pdev, "PME & error events reported via %s\n", pci_name(rcec));

> +
> +	return 0;
> +}
> +
>  /*
>   * pcie_portdrv_probe - Probe PCI-Express port devices
>   * @dev: PCI-Express port device being probed
> @@ -110,6 +122,9 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>  		return -ENODEV;
>  
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> +		pcie_walk_rcec(dev, pcie_hook_rcec, dev);
> +
>  	status = pcie_port_device_register(dev);
>  	if (status)
>  		return status;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c7fc5726872c..ba29816c827b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -330,6 +330,7 @@ struct pci_dev {
>  #ifdef CONFIG_PCIEPORTBUS
>  	u16		rcec_cap;	/* RCEC capability offset */
>  	struct rcec_ext *rcec_ext;	/* RCEC cached assoc. endpoint extended capabilities */
> +	struct pci_dev	*rcec;		/* Associated RCEC device */
>  #endif
>  	u8		pcie_cap;	/* PCIe capability offset */
>  	u8		msi_cap;	/* MSI capability offset */
> -- 
> 2.28.0
>
Sean V Kelley Sept. 2, 2020, 10:24 p.m. UTC | #2
Hi Bjorn,

On Wed, 2020-09-02 at 11:35 -0500, Bjorn Helgaas wrote:
> On Wed, Aug 12, 2020 at 09:46:56AM -0700, Sean V Kelley wrote:
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > 
> > When attempting error recovery for an RCiEP associated with an RCEC
> > device,
> > there needs to be a way to update the Root Error Status, the
> > Uncorrectable
> > Error Status and the Uncorrectable Error Severity of the parent
> > RCEC.
> > So add the 'rcec' field to the pci_dev structure and provide a hook
> > for the
> > Root Port Driver to associate RCiEPs with their respective parent
> > RCEC.
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -202,6 +202,12 @@ pci_ers_result_t pcie_do_recovery(struct
> > pci_dev *dev,
> >  		pci_walk_dev_affected(dev, report_frozen_detected,
> > &status);
> >  		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> >  			status = flr_on_rciep(dev);
> > +			/*
> > +			 * The callback only clears the Root Error
> > Status
> > +			 * of the RCEC (see aer.c).
> > +			 */
> > +			if (dev->rcec)
> > +				reset_link(dev->rcec);
> >  			if (status != PCI_ERS_RESULT_RECOVERED) {
> >  				pci_warn(dev, "function level reset
> > failed\n");
> >  				goto failed;
> > @@ -245,7 +251,11 @@ pci_ers_result_t pcie_do_recovery(struct
> > pci_dev *dev,
> >  	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
> >  		pci_aer_clear_device_status(dev);
> >  		pci_aer_clear_nonfatal_status(dev);
> > +	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END && dev-
> > >rcec) {
> > +		pci_aer_clear_device_status(dev->rcec);
> > +		pci_aer_clear_nonfatal_status(dev->rcec);
> 
> Conceptually, I'm not sure why we need the dev->rcec link.  The error
> was *reported* via the RCEC, so don't we know the RCEC up front,
> before we even identify the RCiEP?  Can't we just remember that and
> dispense with dev->rcec?

However, we can also get errors reported by that same RCEC that are not
related to the associated RCiEPs. Further, an RCiEP in reporting an
error will trigger logging to the Root Error Command/Status and Error
Source Identification registers of the associated RCEC. The assumption
in pcie_do_recovery() here is that I can cover both scenarios.
 
In a new revision of this patch series...

        if (type != PCI_EXP_TYPE_RC_END) {
                if (pcie_aer_is_native(bridge))
                        pcie_clear_device_status(bridge);
                pci_aer_clear_nonfatal_status(bridge);
        }

I've a new revision that makes use of the earlier thread discussing
'bridge' assignment conditionally and when it makes sense to refer to
the 'dev'.

.

> 
> I'm also concerned that if we fail to identify the RCiEP (i.e., we
> don't have a valid "dev" to use dev->rcec), we will fail to clear the
> error status bits.  I think it's possible that the RCEC will report
> an
> error, but the RCiEP that generated the error message is not
> responding so we can't find it.

That's the association which is enumerated at boot by BIOS either
through the bitmap or through the next/last bus range. Are you
describing a scenario in which during enumeration of the RCECs/RCiEPs
the relationships are not discovered?

The second bit you describe is where the error is reported and the
RCiEP is not responding. I wonder if that will eventually trigger an
error from the RCEC.  But you are right such a scenario could happen so
you are getting the error but the device is not present or may not even
have been present from BIOS perspective?

Sean


> 
> >  	}
> > +
> >  	pci_info(dev, "device recovery successful\n");
> >  	return status;
> >  
> > diff --git a/drivers/pci/pcie/portdrv_pci.c
> > b/drivers/pci/pcie/portdrv_pci.c
> > index d5b109499b10..a64e88b7166b 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -90,6 +90,18 @@ static const struct dev_pm_ops
> > pcie_portdrv_pm_ops = {
> >  #define PCIE_PORTDRV_PM_OPS	NULL
> >  #endif /* !PM */
> >  
> > +static int pcie_hook_rcec(struct pci_dev *pdev, void *data)
> > +{
> > +	struct pci_dev *rcec = (struct pci_dev *)data;
> > +
> > +	pdev->rcec = rcec;
> > +	pci_dbg(rcec, "RCiEP(under an RCEC) %04x:%02x:%02x.%d\n",
> > +		pci_domain_nr(pdev->bus), pdev->bus->number,
> > +		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> 
> If we do need dev->rcec, this should use pci_name() for the second
> device instead of formatting the name manually.  I think I would
> connect this with the RCiEP instead of the RCEC, e.g.,
> 
>   pci_dbg(pdev, "PME & error events reported via %s\n",
> pci_name(rcec));
> 
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * pcie_portdrv_probe - Probe PCI-Express port devices
> >   * @dev: PCI-Express port device being probed
> > @@ -110,6 +122,9 @@ static int pcie_portdrv_probe(struct pci_dev
> > *dev,
> >  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
> >  		return -ENODEV;
> >  
> > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> > +		pcie_walk_rcec(dev, pcie_hook_rcec, dev);
> > +
> >  	status = pcie_port_device_register(dev);
> >  	if (status)
> >  		return status;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index c7fc5726872c..ba29816c827b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -330,6 +330,7 @@ struct pci_dev {
> >  #ifdef CONFIG_PCIEPORTBUS
> >  	u16		rcec_cap;	/* RCEC capability offset */
> >  	struct rcec_ext *rcec_ext;	/* RCEC cached assoc. endpoint
> > extended capabilities */
> > +	struct pci_dev	*rcec;		/* Associated RCEC device
> > */
> >  #endif
> >  	u8		pcie_cap;	/* PCIe capability offset */
> >  	u8		msi_cap;	/* MSI capability offset */
> > -- 
> > 2.28.0
> >
diff mbox series

Patch

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 3acf56683915..f1bf06be449e 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1335,17 +1335,18 @@  static int aer_probe(struct pcie_device *dev)
 static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 {
 	int aer = dev->aer_cap;
+	int rc = 0;
 	u32 reg32;
-	int rc;
-
 
 	/* Disable Root's interrupt in response to error messages */
 	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
 	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
 
-	rc = pci_bus_error_reset(dev);
-	pci_info(dev, "Root Port link has been reset\n");
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC) {
+		rc = pci_bus_error_reset(dev);
+		pci_info(dev, "Root Port link has been reset\n");
+	}
 
 	/* Clear Root Error Status */
 	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 04528b57876b..a5b7e708079d 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -202,6 +202,12 @@  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		pci_walk_dev_affected(dev, report_frozen_detected, &status);
 		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
 			status = flr_on_rciep(dev);
+			/*
+			 * The callback only clears the Root Error Status
+			 * of the RCEC (see aer.c).
+			 */
+			if (dev->rcec)
+				reset_link(dev->rcec);
 			if (status != PCI_ERS_RESULT_RECOVERED) {
 				pci_warn(dev, "function level reset failed\n");
 				goto failed;
@@ -245,7 +251,11 @@  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
 		pci_aer_clear_device_status(dev);
 		pci_aer_clear_nonfatal_status(dev);
+	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END && dev->rcec) {
+		pci_aer_clear_device_status(dev->rcec);
+		pci_aer_clear_nonfatal_status(dev->rcec);
 	}
+
 	pci_info(dev, "device recovery successful\n");
 	return status;
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index d5b109499b10..a64e88b7166b 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -90,6 +90,18 @@  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 #define PCIE_PORTDRV_PM_OPS	NULL
 #endif /* !PM */
 
+static int pcie_hook_rcec(struct pci_dev *pdev, void *data)
+{
+	struct pci_dev *rcec = (struct pci_dev *)data;
+
+	pdev->rcec = rcec;
+	pci_dbg(rcec, "RCiEP(under an RCEC) %04x:%02x:%02x.%d\n",
+		pci_domain_nr(pdev->bus), pdev->bus->number,
+		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+	return 0;
+}
+
 /*
  * pcie_portdrv_probe - Probe PCI-Express port devices
  * @dev: PCI-Express port device being probed
@@ -110,6 +122,9 @@  static int pcie_portdrv_probe(struct pci_dev *dev,
 	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
 		return -ENODEV;
 
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
+		pcie_walk_rcec(dev, pcie_hook_rcec, dev);
+
 	status = pcie_port_device_register(dev);
 	if (status)
 		return status;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c7fc5726872c..ba29816c827b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -330,6 +330,7 @@  struct pci_dev {
 #ifdef CONFIG_PCIEPORTBUS
 	u16		rcec_cap;	/* RCEC capability offset */
 	struct rcec_ext *rcec_ext;	/* RCEC cached assoc. endpoint extended capabilities */
+	struct pci_dev	*rcec;		/* Associated RCEC device */
 #endif
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */