From: "Duan, Zhenzhong" <zhenzhong.duan@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
"rafael@kernel.org" <rafael@kernel.org>,
"lenb@kernel.org" <lenb@kernel.org>,
"james.morse@arm.com" <james.morse@arm.com>,
"Luck, Tony" <tony.luck@intel.com>, "bp@alien8.de" <bp@alien8.de>,
"dave@stgolabs.net" <dave@stgolabs.net>,
"Jiang, Dave" <dave.jiang@intel.com>,
"Schofield, Alison" <alison.schofield@intel.com>,
"Verma, Vishal L" <vishal.l.verma@intel.com>,
"Weiny, Ira" <ira.weiny@intel.com>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"helgaas@kernel.org" <helgaas@kernel.org>,
"mahesh@linux.ibm.com" <mahesh@linux.ibm.com>,
"oohall@gmail.com" <oohall@gmail.com>,
"linmiaohe@huawei.com" <linmiaohe@huawei.com>,
"shiju.jose@huawei.com" <shiju.jose@huawei.com>,
"Preble, Adam C" <adam.c.preble@intel.com>,
"leoyang.li@nxp.com" <leoyang.li@nxp.com>,
"lukas@wunner.de" <lukas@wunner.de>,
"Smita.KoralahalliChannabasappa@amd.com"
<Smita.KoralahalliChannabasappa@amd.com>,
"rrichter@amd.com" <rrichter@amd.com>,
"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Tsaur, Erwin" <erwin.tsaur@intel.com>,
"Kuppuswamy,
Sathyanarayanan" <sathyanarayanan.kuppuswamy@intel.com>,
"Williams, Dan J" <dan.j.williams@intel.com>,
"Wanyan, Feiting" <feiting.wanyan@intel.com>,
"Wang, Yudong" <yudong.wang@intel.com>,
"Peng, Chao P" <chao.p.peng@intel.com>,
"qingshun.wang@linux.intel.com" <qingshun.wang@linux.intel.com>
Subject: RE: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info
Date: Sun, 28 Apr 2024 03:31:11 +0000 [thread overview]
Message-ID: <SJ0PR11MB6744C3EAA6E9D738EA0D3BAC92142@SJ0PR11MB6744.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20240426171158.000024d4@Huawei.com>
Hi Jonathan,
>-----Original Message-----
>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>Subject: Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might
>be ANFE in aer_err_info
>
>On Tue, 23 Apr 2024 02:25:05 +0000
>"Duan, Zhenzhong" <zhenzhong.duan@intel.com> wrote:
>
>> >-----Original Message-----
>> >From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> >Subject: Re: [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that
>might
>> >be ANFE in aer_err_info
>> >
>> >On Wed, 17 Apr 2024 14:14:05 +0800
>> >Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
>> >
>> >> In some cases the detector of a Non-Fatal Error(NFE) is not the most
>> >> appropriate agent to determine the type of the error. For example,
>> >> when software performs a configuration read from a non-existent
>> >> device or Function, completer will send an ERR_NONFATAL Message.
>> >> On some platforms, ERR_NONFATAL results in a System Error, which
>> >> breaks normal software probing.
>> >>
>> >> Advisory Non-Fatal Error(ANFE) is a special case that can be used
>> >> in above scenario. It is predominantly determined by the role of the
>> >> detecting agent (Requester, Completer, or Receiver) and the specific
>> >> error. In such cases, an agent with AER signals the NFE (if enabled)
>> >> by sending an ERR_COR Message as an advisory to software, instead of
>> >> sending ERR_NONFATAL.
>> >>
>> >> When processing an ANFE, ideally both correctable error(CE) status and
>> >> uncorrectable error(UE) status should be cleared. However, there is no
>> >> way to fully identify the UE associated with ANFE. Even worse, a Fatal
>> >> Error(FE) or Non-Fatal Error(NFE) may set the same UE status bit as
>> >> ANFE. Treating an ANFE as NFE will reproduce above mentioned issue,
>> >> i.e., breaking softwore probing; treating NFE as ANFE will make us
>> >> ignoring some UEs which need active recover operation. To avoid
>clearing
>> >> UEs that are not ANFE by accident, the most conservative route is taken
>> >> here: If any of the FE/NFE Detected bits is set in Device Status, do not
>> >> touch UE status, they should be cleared later by the UE handler.
>Otherwise,
>> >> a specific set of UEs that may be raised as ANFE according to the PCIe
>> >> specification will be cleared if their corresponding severity is Non-Fatal.
>> >>
>> >> To achieve above purpose, store UNCOR_STATUS bits that might be
>ANFE
>> >> in aer_err_info.anfe_status. So that those bits could be printed and
>> >> processed later.
>> >>
>> >> Tested-by: Yudong Wang <yudong.wang@intel.com>
>> >> Co-developed-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
>> >> Signed-off-by: "Wang, Qingshun" <qingshun.wang@linux.intel.com>
>> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> >> ---
>> >> drivers/pci/pci.h | 1 +
>> >> drivers/pci/pcie/aer.c | 45
>> >++++++++++++++++++++++++++++++++++++++++++
>> >> 2 files changed, 46 insertions(+)
>> >>
>> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> >> index 17fed1846847..3f9eb807f9fd 100644
>> >> --- a/drivers/pci/pci.h
>> >> +++ b/drivers/pci/pci.h
>> >> @@ -412,6 +412,7 @@ struct aer_err_info {
>> >>
>> >> unsigned int status; /* COR/UNCOR Error Status */
>> >> unsigned int mask; /* COR/UNCOR Error Mask */
>> >> + unsigned int anfe_status; /* UNCOR Error Status for ANFE */
>> >> struct pcie_tlp_log tlp; /* TLP Header */
>> >> };
>> >>
>> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> >> index ac6293c24976..27364ab4b148 100644
>> >> --- a/drivers/pci/pcie/aer.c
>> >> +++ b/drivers/pci/pcie/aer.c
>> >> @@ -107,6 +107,12 @@ struct aer_stats {
>> >> PCI_ERR_ROOT_MULTI_COR_RCV |
>> > \
>> >> PCI_ERR_ROOT_MULTI_UNCOR_RCV)
>> >>
>> >> +#define AER_ERR_ANFE_UNC_MASK
>> > (PCI_ERR_UNC_POISON_TLP | \
>> >> + PCI_ERR_UNC_COMP_TIME |
>> > \
>> >> + PCI_ERR_UNC_COMP_ABORT |
>> > \
>> >> + PCI_ERR_UNC_UNX_COMP |
>> > \
>> >> + PCI_ERR_UNC_UNSUP)
>> >> +
>> >> static int pcie_aer_disable;
>> >> static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>> >>
>> >> @@ -1196,6 +1202,41 @@ void aer_recover_queue(int domain,
>unsigned
>> >int bus, unsigned int devfn,
>> >> EXPORT_SYMBOL_GPL(aer_recover_queue);
>> >> #endif
>> >>
>> >> +static void anfe_get_uc_status(struct pci_dev *dev, struct
>aer_err_info
>> >*info)
>> >> +{
>> >> + u32 uncor_mask, uncor_status;
>> >> + u16 device_status;
>> >> + int aer = dev->aer_cap;
>> >> +
>> >> + if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA,
>> >&device_status))
>> >> + return;
>> >> + /*
>> >> + * Take the most conservative route here. If there are
>> >> + * Non-Fatal/Fatal errors detected, do not assume any
>> >> + * bit in uncor_status is set by ANFE.
>> >> + */
>> >> + if (device_status & (PCI_EXP_DEVSTA_NFED | PCI_EXP_DEVSTA_FED))
>> >> + return;
>> >> +
>> >
>> >Is there not a race here? If we happen to get either an NFED or FED
>> >between the read of device_status above and here we might pick up a
>status
>> >that corresponds to that (and hence clear something we should not).
>>
>> In this scenario, info->anfe_status is 0.
>
>OK. In that case what is the point of the check above?
>If the code is safe to races, it's safe to go ahead without that check
>on what might race.
Good question.
After further digging into the spec, I just found I misunderstood it.
An UNCUR error raised as ANFE can be raised as NFE in different cases,
so info->anfe_status can be nonzero here and the race you mentioned
does exist, the check on PCI_EXP_DEVSTA_FED is also unnecessary.
Sorry for the misleading. I plan to have below change to fix the race:
unsigned int anfe_status;
anfe_status = uncor_status & ~uncor_mask & ~info->severity &
AER_ERR_ANFE_UNC_MASK;
if (pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &device_status))
return;
/*
* Take the most conservative route here. If there are
* Non-Fatal errors detected, do not assume any
* bit in uncor_status is set by ANFE.
*/
if (device_status & PCI_EXP_DEVSTA_NFED)
return;
info->anfe_status = anfe_status;
With this change, there is still a small window between reading uncor_status
and device_status to leak ANFE, but that's the best we can do and better
than clearing NFE. Let me know if you have better idea😊
Thanks
Zhenzhong
>
>>
>> >
>> >Or am I missing that race being close somewhere?
>>
>> The bits leading to NFED or FED is masked out when assigning info-
>>anfe_status.
>> Bits for FED is masked out by ~info->severity,
>> bit for NFED is masked out by AER_ERR_ANFE_UNC_MASK.
>>
>> So we never clear status bits for NFED or FED in ANFE handler.
>>
>> See below assignment of info->anfe_status.
>>
>> Thanks
>> Zhenzhong
>>
>> >
>> >> + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
>> >&uncor_status);
>> >> + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
>> >&uncor_mask);
>> >> + /*
>> >> + * According to PCIe Base Specification Revision 6.1,
>> >> + * Section 6.2.3.2.4, if an UNCOR error is raised as
>> >> + * Advisory Non-Fatal error, it will match the following
>> >> + * conditions:
>> >> + * a. The severity of the error is Non-Fatal.
>> >> + * b. The error is one of the following:
>> >> + * 1. Poisoned TLP (Section 6.2.3.2.4.3)
>> >> + * 2. Completion Timeout (Section 6.2.3.2.4.4)
>> >> + * 3. Completer Abort (Section 6.2.3.2.4.1)
>> >> + * 4. Unexpected Completion (Section 6.2.3.2.4.5)
>> >> + * 5. Unsupported Request (Section 6.2.3.2.4.1)
>> >> + */
>> >> + info->anfe_status = uncor_status & ~uncor_mask & ~info->severity
>> >&
>> >> + AER_ERR_ANFE_UNC_MASK;
>> >> +}
>> >> +
>> >> /**
>> >> * aer_get_device_error_info - read error status from dev and store it
>to
>> >info
>> >> * @dev: pointer to the device expected to have a error record
>> >> @@ -1213,6 +1254,7 @@ int aer_get_device_error_info(struct pci_dev
>> >*dev, struct aer_err_info *info)
>> >>
>> >> /* Must reset in this function */
>> >> info->status = 0;
>> >> + info->anfe_status = 0;
>> >> info->tlp_header_valid = 0;
>> >>
>> >> /* The device might not support AER */
>> >> @@ -1226,6 +1268,9 @@ int aer_get_device_error_info(struct pci_dev
>> >*dev, struct aer_err_info *info)
>> >> &info->mask);
>> >> if (!(info->status & ~info->mask))
>> >> return 0;
>> >> +
>> >> + if (info->status & PCI_ERR_COR_ADV_NFAT)
>> >> + anfe_get_uc_status(dev, info);
>> >> } else if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> >> type == PCI_EXP_TYPE_RC_EC ||
>> >> type == PCI_EXP_TYPE_DOWNSTREAM ||
>>
>>
next prev parent reply other threads:[~2024-04-28 3:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-17 6:14 [PATCH v3 0/3] PCI/AER: Handle Advisory Non-Fatal error Zhenzhong Duan
2024-04-17 6:14 ` [PATCH v3 1/3] PCI/AER: Store UNCOR_STATUS bits that might be ANFE in aer_err_info Zhenzhong Duan
2024-04-22 16:16 ` Jonathan Cameron
2024-04-23 2:25 ` Duan, Zhenzhong
2024-04-26 16:11 ` Jonathan Cameron
2024-04-28 3:31 ` Duan, Zhenzhong [this message]
2024-05-01 12:24 ` Jonathan Cameron
2024-05-03 11:10 ` Duan, Zhenzhong
2024-04-17 6:14 ` [PATCH v3 2/3] PCI/AER: Print UNCOR_STATUS bits that might be ANFE Zhenzhong Duan
2024-04-17 6:14 ` [PATCH v3 3/3] PCI/AER: Clear " Zhenzhong Duan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SJ0PR11MB6744C3EAA6E9D738EA0D3BAC92142@SJ0PR11MB6744.namprd11.prod.outlook.com \
--to=zhenzhong.duan@intel.com \
--cc=Jonathan.Cameron@Huawei.com \
--cc=Smita.KoralahalliChannabasappa@amd.com \
--cc=adam.c.preble@intel.com \
--cc=alison.schofield@intel.com \
--cc=bhelgaas@google.com \
--cc=bp@alien8.de \
--cc=chao.p.peng@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=erwin.tsaur@intel.com \
--cc=feiting.wanyan@intel.com \
--cc=helgaas@kernel.org \
--cc=ira.weiny@intel.com \
--cc=james.morse@arm.com \
--cc=lenb@kernel.org \
--cc=leoyang.li@nxp.com \
--cc=linmiaohe@huawei.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lukas@wunner.de \
--cc=mahesh@linux.ibm.com \
--cc=oohall@gmail.com \
--cc=qingshun.wang@linux.intel.com \
--cc=rafael@kernel.org \
--cc=rrichter@amd.com \
--cc=sathyanarayanan.kuppuswamy@intel.com \
--cc=shiju.jose@huawei.com \
--cc=tony.luck@intel.com \
--cc=vishal.l.verma@intel.com \
--cc=yudong.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).