Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

["Kelley, Sean V" <sean.v.kelley@intel.com>]

> On Dec 2, 2020, at 3:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Fri, Nov 20, 2020 at 04:10:33PM -0800, 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.  In
>> some non-native cases in which there is no OS-visible device associated
>> with the RCiEP, there is nothing to act upon as the firmware is acting
>> before the OS.
>> 
>> Add handling for the linked RCEC in AER/ERR while taking into account
>> non-native cases.
>> 
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>> drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
>> drivers/pci/pcie/err.c | 20 +++++++++---------
>> 2 files changed, 44 insertions(+), 22 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 0ba0b47ae751..51389a6ee4ca 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
>> +	struct pci_dev *root;
>> +	int aer = 0;
>> +	int rc = 0;
>> 	u32 reg32;
>> -	int rc;
>> 
>> -	if (pcie_aer_is_native(dev)) {
>> +	if (type == PCI_EXP_TYPE_RC_END)
>> +		/*
>> +		 * The reset should only clear the Root Error Status
>> +		 * of the RCEC. Only perform this for the
>> +		 * native case, i.e., an RCEC is present.
>> +		 */
>> +		root = dev->rcec;
>> +	else
>> +		root = dev;
>> +
>> +	if (root)
>> +		aer = dev->aer_cap;
>> +
>> +	if ((aer) && pcie_aer_is_native(dev)) {
>> 		/* Disable Root's interrupt in response to error messages */
>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> 	}
>> 
>> -	rc = pci_bus_error_reset(dev);
>> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>> +		if (pcie_has_flr(dev)) {
>> +			rc = pcie_flr(dev);
>> +			pci_info(dev, "has been reset (%d)\n", rc);
> 
> Maybe:
> 
>  +             } else {
>  +                     rc = -ENOTTY;
>  +                     pci_info(dev, "not reset (no FLR support)\n");
> 
> Or do we want to pretend the device was reset and return
> PCI_ERS_RESULT_RECOVERED?

We are currently doing the latter now with the default of rc = 0 above and so  I’m not sure the extra detail here on the absence of FLR support is of value.


> 
>> +	} else {
>> +		rc = pci_bus_error_reset(dev);
>> +		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>> +	}
>> 
>> -	if (pcie_aer_is_native(dev)) {
>> +	if ((aer) && pcie_aer_is_native(dev)) {
>> 		/* Clear Root Error Status */
>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>> 
>> 		/* Enable Root Port's interrupt in response to error messages */
>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> 		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> 	}
>> 
>> 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 7883c9791562..cbc5abfe767b 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -148,10 +148,10 @@ static int report_resume(struct pci_dev *dev, void *data)
>> 
>> /**
>>  * pci_walk_bridge - walk bridges potentially AER affected
>> - * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
>> - *		or an RCiEP associated with an RCEC
>> - * @cb:		callback to be called for each device found
>> - * @userdata:	arbitrary pointer to be passed to callback
>> + * @bridge   bridge which may be an RCEC with associated RCiEPs,
>> + *           or a Port.
>> + * @cb       callback to be called for each device found
>> + * @userdata arbitrary pointer to be passed to callback.
>>  *
>>  * If the device provided is a bridge, walk the subordinate bus, including
>>  * any bridged devices on buses under this bus.  Call the provided callback
>> @@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>> 			    int (*cb)(struct pci_dev *, void *),
>> 			    void *userdata)
>> {
>> +	/*
>> +	 * In a non-native case where there is no OS-visible reporting
>> +	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
> 
> I don't quite understand this comment.  I see that in the non-native
> case, the reporting device may not be OS-visible.  But I don't
> understand why the comment is *here*.
> 
> If "bridge" can be NULL here, we should test that before dereferencing
> "bridge->subordinate".

Wrongly worded.  The subordinate may be NULL or the associated RCEC may be NULL, not the “bridge”.
However, per below, we should not be trying to call report_frozen_detected(), report_mmio_enabled() via
the associated RCEC’s driver, but rather the CB for the RCiEP itself.

Going back to this conversation,

https://lore.kernel.org/linux-pci/20201016172210.GA86168@bjorn-Precision-5520/

"Looks like *this* is the patch where the "no subordinate bus" case
becomes possible?  If you agree, I can just move the test here, no
need to repost.”

It is actually the case we are only dealing with the absence of a subordinate bus.

> 
>> 	if (bridge->subordinate)
>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>> +	else if (bridge->rcec)
>> +		cb(bridge->rcec, userdata);
> 
> And I don't understand what's going on here.  In this case, I *think*
> "bridge" is an RCiEP and "bridge->rcec" is the related RCEC, so it
> looks like we'll call report_frozen_detected(), report_mmio_enabled(),
> etc for the RCEC driver.  I would think we'd want the RCiEP driver.

Indeed, the bridge->rcec here is the dev->rcec in which the dev is the RCiEP.

And we don’t need that conditional here, it should just hit the device driver’s routines.

This is an unfortunate side effect of the RCiEP being subordinate to the RCEC but for
 purposes of linking, it gives the impression of the other way around.


> 
> Sorry if I'm missing the obvious.

Actually your observations are on point.

Thanks,

Sean

> 
>> 	else
>> 		cb(bridge, userdata);
>> }
>> @@ -194,12 +200,6 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 	pci_dbg(bridge, "broadcast error_detected message\n");
>> 	if (state == pci_channel_io_frozen) {
>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
>> -		if (type == PCI_EXP_TYPE_RC_END) {
>> -			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
>> -			status = PCI_ERS_RESULT_NONE;
>> -			goto failed;
>> -		}
>> -
>> 		status = reset_subordinates(bridge);
>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
>> 			pci_warn(bridge, "subordinate device reset failed\n");
>> -- 
>> 2.29.2
>>