* [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 10:00 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC Sean V Kelley
` (8 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
A PCIe Root Complex Event Collector(RCEC) has the base class 0x08,
sub-class 0x07, and programming interface 0x00. Add the class code
0x0807 to identify RCEC devices and add the defines for the RCEC
Endpoint Association Extended Capability.
See PCI Express Base Specification, version 5.0-1, section "1.3.4
Root Complex Event Collector" and section "7.9.10 Root Complex
Event Collector Endpoint Association Extended Capability"
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
include/linux/pci_ids.h | 1 +
include/uapi/linux/pci_regs.h | 7 +++++++
2 files changed, 8 insertions(+)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 0ad57693f392..de8dff1fb176 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -81,6 +81,7 @@
#define PCI_CLASS_SYSTEM_RTC 0x0803
#define PCI_CLASS_SYSTEM_PCI_HOTPLUG 0x0804
#define PCI_CLASS_SYSTEM_SDHCI 0x0805
+#define PCI_CLASS_SYSTEM_RCEC 0x0807
#define PCI_CLASS_SYSTEM_OTHER 0x0880
#define PCI_BASE_CLASS_INPUT 0x09
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index f9701410d3b5..f335f65f65d6 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -828,6 +828,13 @@
#define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */
#define PCI_EXT_CAP_PWR_SIZEOF 16
+/* Root Complex Event Collector Endpoint Association */
+#define PCI_RCEC_RCIEP_BITMAP 4 /* Associated Bitmap for RCiEPs */
+#define PCI_RCEC_BUSN 8 /* RCEC Associated Bus Numbers */
+#define PCI_RCEC_BUSN_REG_VER 0x02 /* Least capability version that BUSN present */
+#define PCI_RCEC_BUSN_NEXT(x) (((x) >> 8) & 0xff)
+#define PCI_RCEC_BUSN_LAST(x) (((x) >> 16) & 0xff)
+
/* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
#define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */
#define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff)
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC
2020-07-24 17:22 ` [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC Sean V Kelley
@ 2020-07-27 10:00 ` Jonathan Cameron
2020-07-27 10:21 ` Jonathan Cameron
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 10:00 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Fri, 24 Jul 2020 10:22:15 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>
> A PCIe Root Complex Event Collector(RCEC) has the base class 0x08,
> sub-class 0x07, and programming interface 0x00. Add the class code
> 0x0807 to identify RCEC devices and add the defines for the RCEC
> Endpoint Association Extended Capability.
>
> See PCI Express Base Specification, version 5.0-1, section "1.3.4
> Root Complex Event Collector" and section "7.9.10 Root Complex
> Event Collector Endpoint Association Extended Capability"
Add a reference to the document
"PCI Code and ID Assignment Specification"
for the class number.
From the change log on latest version seems like it's been there since
version 1.4.
There is a worrying note (bottom of page 16 of 1.12 version of that docs)
in there that says some older specs used 0x0806 for RCECs and that we
should use the port type field to actually check if we have one.
Hopefully we won't encounter any of those in the wild.
Otherwise, it's exactly what the spec says.
We could bike shed on naming choices, but the ones you have seem clear enough
to me.
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Jonathan
>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> include/linux/pci_ids.h | 1 +
> include/uapi/linux/pci_regs.h | 7 +++++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 0ad57693f392..de8dff1fb176 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -81,6 +81,7 @@
> #define PCI_CLASS_SYSTEM_RTC 0x0803
> #define PCI_CLASS_SYSTEM_PCI_HOTPLUG 0x0804
> #define PCI_CLASS_SYSTEM_SDHCI 0x0805
> +#define PCI_CLASS_SYSTEM_RCEC 0x0807
> #define PCI_CLASS_SYSTEM_OTHER 0x0880
>
> #define PCI_BASE_CLASS_INPUT 0x09
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index f9701410d3b5..f335f65f65d6 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -828,6 +828,13 @@
> #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */
> #define PCI_EXT_CAP_PWR_SIZEOF 16
>
> +/* Root Complex Event Collector Endpoint Association */
> +#define PCI_RCEC_RCIEP_BITMAP 4 /* Associated Bitmap for RCiEPs */
> +#define PCI_RCEC_BUSN 8 /* RCEC Associated Bus Numbers */
> +#define PCI_RCEC_BUSN_REG_VER 0x02 /* Least capability version that BUSN present */
> +#define PCI_RCEC_BUSN_NEXT(x) (((x) >> 8) & 0xff)
> +#define PCI_RCEC_BUSN_LAST(x) (((x) >> 16) & 0xff)
> +
> /* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
> #define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */
> #define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff)
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC
2020-07-27 10:00 ` Jonathan Cameron
@ 2020-07-27 10:21 ` Jonathan Cameron
2020-07-27 15:22 ` Sean V Kelley
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 10:21 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Mon, 27 Jul 2020 11:00:10 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> On Fri, 24 Jul 2020 10:22:15 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >
> > A PCIe Root Complex Event Collector(RCEC) has the base class 0x08,
> > sub-class 0x07, and programming interface 0x00. Add the class code
> > 0x0807 to identify RCEC devices and add the defines for the RCEC
> > Endpoint Association Extended Capability.
> >
> > See PCI Express Base Specification, version 5.0-1, section "1.3.4
> > Root Complex Event Collector" and section "7.9.10 Root Complex
> > Event Collector Endpoint Association Extended Capability"
>
> Add a reference to the document
> "PCI Code and ID Assignment Specification"
> for the class number.
Actually probably no need. I'd somehow managed to fail to notice the
class code is also given in section 1.3.4 of the main spec.
>
> From the change log on latest version seems like it's been there since
> version 1.4.
>
> There is a worrying note (bottom of page 16 of 1.12 version of that docs)
> in there that says some older specs used 0x0806 for RCECs and that we
> should use the port type field to actually check if we have one.
>
> Hopefully we won't encounter any of those in the wild.
>
> Otherwise, it's exactly what the spec says.
> We could bike shed on naming choices, but the ones you have seem clear enough
> to me.
>
> FWIW
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>
> Jonathan
> >
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > ---
> > include/linux/pci_ids.h | 1 +
> > include/uapi/linux/pci_regs.h | 7 +++++++
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index 0ad57693f392..de8dff1fb176 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -81,6 +81,7 @@
> > #define PCI_CLASS_SYSTEM_RTC 0x0803
> > #define PCI_CLASS_SYSTEM_PCI_HOTPLUG 0x0804
> > #define PCI_CLASS_SYSTEM_SDHCI 0x0805
> > +#define PCI_CLASS_SYSTEM_RCEC 0x0807
> > #define PCI_CLASS_SYSTEM_OTHER 0x0880
> >
> > #define PCI_BASE_CLASS_INPUT 0x09
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index f9701410d3b5..f335f65f65d6 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -828,6 +828,13 @@
> > #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system budget */
> > #define PCI_EXT_CAP_PWR_SIZEOF 16
> >
> > +/* Root Complex Event Collector Endpoint Association */
> > +#define PCI_RCEC_RCIEP_BITMAP 4 /* Associated Bitmap for RCiEPs */
> > +#define PCI_RCEC_BUSN 8 /* RCEC Associated Bus Numbers */
> > +#define PCI_RCEC_BUSN_REG_VER 0x02 /* Least capability version that BUSN present */
> > +#define PCI_RCEC_BUSN_NEXT(x) (((x) >> 8) & 0xff)
> > +#define PCI_RCEC_BUSN_LAST(x) (((x) >> 16) & 0xff)
> > +
> > /* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
> > #define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */
> > #define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC
2020-07-27 10:21 ` Jonathan Cameron
@ 2020-07-27 15:22 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 15:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, Raj, Ashok, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On 27 Jul 2020, at 3:21, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 11:00:10 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
>> On Fri, 24 Jul 2020 10:22:15 -0700
>> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>>
>>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>>
>>> A PCIe Root Complex Event Collector(RCEC) has the base class 0x08,
>>> sub-class 0x07, and programming interface 0x00. Add the class code
>>> 0x0807 to identify RCEC devices and add the defines for the RCEC
>>> Endpoint Association Extended Capability.
>>>
>>> See PCI Express Base Specification, version 5.0-1, section "1.3.4
>>> Root Complex Event Collector" and section "7.9.10 Root Complex
>>> Event Collector Endpoint Association Extended Capability"
>>
>> Add a reference to the document
>> "PCI Code and ID Assignment Specification"
>> for the class number.
>
> Actually probably no need. I'd somehow managed to fail to notice the
> class code is also given in section 1.3.4 of the main spec.
Okay, will leave as is.
Thanks,
Sean
>
>>
>> From the change log on latest version seems like it's been there
>> since
>> version 1.4.
>>
>> There is a worrying note (bottom of page 16 of 1.12 version of that
>> docs)
>> in there that says some older specs used 0x0806 for RCECs and that we
>> should use the port type field to actually check if we have one.
>>
>> Hopefully we won't encounter any of those in the wild.
>>
>> Otherwise, it's exactly what the spec says.
>> We could bike shed on naming choices, but the ones you have seem
>> clear enough
>> to me.
>>
>> FWIW
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>>
>> Jonathan
>>>
>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>> ---
>>> include/linux/pci_ids.h | 1 +
>>> include/uapi/linux/pci_regs.h | 7 +++++++
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>>> index 0ad57693f392..de8dff1fb176 100644
>>> --- a/include/linux/pci_ids.h
>>> +++ b/include/linux/pci_ids.h
>>> @@ -81,6 +81,7 @@
>>> #define PCI_CLASS_SYSTEM_RTC 0x0803
>>> #define PCI_CLASS_SYSTEM_PCI_HOTPLUG 0x0804
>>> #define PCI_CLASS_SYSTEM_SDHCI 0x0805
>>> +#define PCI_CLASS_SYSTEM_RCEC 0x0807
>>> #define PCI_CLASS_SYSTEM_OTHER 0x0880
>>>
>>> #define PCI_BASE_CLASS_INPUT 0x09
>>> diff --git a/include/uapi/linux/pci_regs.h
>>> b/include/uapi/linux/pci_regs.h
>>> index f9701410d3b5..f335f65f65d6 100644
>>> --- a/include/uapi/linux/pci_regs.h
>>> +++ b/include/uapi/linux/pci_regs.h
>>> @@ -828,6 +828,13 @@
>>> #define PCI_PWR_CAP_BUDGET(x) ((x) & 1) /* Included in system
>>> budget */
>>> #define PCI_EXT_CAP_PWR_SIZEOF 16
>>>
>>> +/* Root Complex Event Collector Endpoint Association */
>>> +#define PCI_RCEC_RCIEP_BITMAP 4 /* Associated Bitmap for RCiEPs */
>>> +#define PCI_RCEC_BUSN 8 /* RCEC Associated Bus Numbers */
>>> +#define PCI_RCEC_BUSN_REG_VER 0x02 /* Least capability version
>>> that BUSN present */
>>> +#define PCI_RCEC_BUSN_NEXT(x) (((x) >> 8) & 0xff)
>>> +#define PCI_RCEC_BUSN_LAST(x) (((x) >> 16) & 0xff)
>>> +
>>> /* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
>>> #define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */
>>> #define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff)
>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
2020-07-24 17:22 ` [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 12:30 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC Sean V Kelley
` (7 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo, Sean V Kelley
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
optionally be sent to a corresponding Root Complex Event Collector (RCEC).
Each RCiEP must be associated with no more than one RCEC. Interface errors
are reported to the OS by RCECs.
For an RCEC (technically not a Bridge), error messages "received" from
associated RCiEPs must be enabled for "transmission" in order to cause a
System Error via the Root Control register or (when the Advanced Error
Reporting Capability is present) reporting via the Root Error Command
register and logging in the Root Error Status register and Error Source
Identification register.
Given the commonality with Root Ports and the need to also support AER
and PME services for RCECs, extend the Root Port driver to support RCEC
devices through the addition of the RCEC Class ID to the driver
structure.
Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
drivers/pci/pcie/portdrv_pci.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 3acf151ae015..d5b109499b10 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
if (!pci_is_pcie(dev) ||
((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
(pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
- (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
+ (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
+ (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
return -ENODEV;
status = pcie_port_device_register(dev);
@@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
+ /* handle any Root Complex Event Collector */
+ { PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
{ },
};
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC
2020-07-24 17:22 ` [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC Sean V Kelley
@ 2020-07-27 12:30 ` Jonathan Cameron
2020-07-27 15:05 ` Sean V Kelley
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 12:30 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Fri, 24 Jul 2020 10:22:16 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>
> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
> optionally be sent to a corresponding Root Complex Event Collector (RCEC).
> Each RCiEP must be associated with no more than one RCEC. Interface errors
> are reported to the OS by RCECs.
>
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
>
> Given the commonality with Root Ports and the need to also support AER
> and PME services for RCECs, extend the Root Port driver to support RCEC
> devices through the addition of the RCEC Class ID to the driver
> structure.
>
Hi.
I'm surprised it ended up this simple :) Seems we are a bit lucky that
the existing code is rather flexible on what is there and what isn't
and that there is never any reason to directly touch the various
type1 specific registers (given as I read the spec, an RCEC uses a
type0 config space header unlike the ports).
Given you mention PME, it's probably worth noting (I think) you aren't
actually enabling the service yet as there is a check in that path on whether we
have a root port or not.
https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/pci/pcie/portdrv_core.c#L241
Thanks,
Jonathan
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
> drivers/pci/pcie/portdrv_pci.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 3acf151ae015..d5b109499b10 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> if (!pci_is_pcie(dev) ||
> ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
> (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
> - (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
> + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
> + (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
> return -ENODEV;
>
> status = pcie_port_device_register(dev);
> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
> { PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
> /* subtractive decode PCI-to-PCI bridge, class type is 060401h */
> { PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
> + /* handle any Root Complex Event Collector */
> + { PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
> { },
> };
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC
2020-07-27 12:30 ` Jonathan Cameron
@ 2020-07-27 15:05 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 15:05 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, tony.luck, Raj, Ashok, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On 27 Jul 2020, at 5:30, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:16 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors
>> may
>> optionally be sent to a corresponding Root Complex Event Collector
>> (RCEC).
>> Each RCiEP must be associated with no more than one RCEC. Interface
>> errors
>> are reported to the OS by RCECs.
>>
>> For an RCEC (technically not a Bridge), error messages "received"
>> from
>> associated RCiEPs must be enabled for "transmission" in order to
>> cause a
>> System Error via the Root Control register or (when the Advanced
>> Error
>> Reporting Capability is present) reporting via the Root Error Command
>> register and logging in the Root Error Status register and Error
>> Source
>> Identification register.
>>
>> Given the commonality with Root Ports and the need to also support
>> AER
>> and PME services for RCECs, extend the Root Port driver to support
>> RCEC
>> devices through the addition of the RCEC Class ID to the driver
>> structure.
>>
> Hi.
>
> I'm surprised it ended up this simple :) Seems we are a bit lucky that
> the existing code is rather flexible on what is there and what isn't
> and that there is never any reason to directly touch the various
> type1 specific registers (given as I read the spec, an RCEC uses a
> type0 config space header unlike the ports).
Which is part of the reason why I chose to refer to it as an RFC,
because it seemed simpler and I was unsure if we were missing anything,
and it turns out we were. Unfortunately, to avoid churn, I’ve left
quite a bit of comments/naming intact with “root”/“port” terms.
>
> Given you mention PME, it's probably worth noting (I think) you aren't
> actually enabling the service yet as there is a check in that path on
> whether we
> have a root port or not.
> https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/pci/pcie/portdrv_core.c#L241
Good catch, testing has been only done at this point with AER injection.
Will correct.
Thanks,
Sean
>
> Thanks,
>
> Jonathan
>
>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> ---
>> drivers/pci/pcie/portdrv_pci.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv_pci.c
>> b/drivers/pci/pcie/portdrv_pci.c
>> index 3acf151ae015..d5b109499b10 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev
>> *dev,
>> if (!pci_is_pcie(dev) ||
>> ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>> (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
>> - (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
>> + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
>> + (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>> return -ENODEV;
>>
>> status = pcie_port_device_register(dev);
>> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[]
>> = {
>> { PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>> /* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>> { PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
>> + /* handle any Root Complex Event Collector */
>> + { PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>> { },
>> };
>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
2020-07-24 17:22 ` [RFC PATCH 1/9] pci_ids: Add class code and extended capability for RCEC Sean V Kelley
2020-07-24 17:22 ` [RFC PATCH 2/9] PCI: Extend Root Port Driver to support RCEC Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 10:49 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs Sean V Kelley
` (6 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo, Sean V Kelley
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
When an RCEC device signals error(s) to a CPU core, the CPU core
needs to walk all the RCiEPs associated with that RCEC to check
errors. So add the function pcie_walk_rcec() to walk all RCiEPs
associated with the RCEC device.
Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
drivers/pci/pcie/portdrv.h | 2 +
drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+)
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index af7cf237432a..c11d5ecbad76 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct pcie_port_service_driver *new);
extern struct bus_type pcie_port_bus_type;
int pcie_port_device_register(struct pci_dev *dev);
+void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
+ void *userdata);
#ifdef CONFIG_PM
int pcie_port_device_suspend(struct device *dev);
int pcie_port_device_resume_noirq(struct device *dev);
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522ab07d..bdcbb34764c2 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -14,6 +14,7 @@
#include <linux/pm_runtime.h>
#include <linux/string.h>
#include <linux/slab.h>
+#include <linux/bitops.h>
#include <linux/aer.h>
#include "../pci.h"
@@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev *dev)
return status;
}
+static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int (*cb)(struct pci_dev *, void *),
+ void *userdata, unsigned long bitmap)
+{
+ unsigned int dev, fn;
+ struct pci_dev *pdev;
+ int retval;
+
+ for_each_set_bit(dev, &bitmap, 32) {
+ for (fn = 0; fn < 8; fn++) {
+ pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
+
+ if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
+ continue;
+
+ retval = cb(pdev, userdata);
+ if (retval)
+ return retval;
+ }
+ }
+
+ return 0;
+}
+
+/** pcie_walk_rcec - Walk RCiEP devices associating with RCEC and call callback.
+ * @rcec RCEC whose RCiEP devices should be walked.
+ * @cb Callback to be called for each RCiEP device found.
+ * @userdata Arbitrary pointer to be passed to callback.
+ *
+ * Walk the given RCEC. Call the provided callback on each RCiEP device found.
+ *
+ * We check the return of @cb each time. If it returns anything
+ * other than 0, we break out.
+ *
+ */
+void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
+ void *userdata)
+{
+ u32 pos, bitmap, hdr, busn;
+ u8 ver, nextbusn, lastbusn;
+ struct pci_bus *pbus;
+ unsigned int bnr;
+
+ pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
+ if (!pos)
+ return;
+
+ pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
+ if (!pbus)
+ return;
+
+ pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
+
+ /* Find RCiEP devices on the same bus as the RCEC */
+ if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned long)bitmap))
+ return;
+
+ /* Check whether RCEC BUSN register is present */
+ pci_read_config_dword(rcec, pos, &hdr);
+ ver = PCI_EXT_CAP_VER(hdr);
+ if (ver < PCI_RCEC_BUSN_REG_VER)
+ return;
+
+ pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
+ nextbusn = PCI_RCEC_BUSN_NEXT(busn);
+ lastbusn = PCI_RCEC_BUSN_LAST(busn);
+
+ /* All RCiEP devices are on the same bus as the RCEC */
+ if (nextbusn == 0xff && lastbusn == 0x00)
+ return;
+
+ for (bnr = nextbusn; bnr < (lastbusn + 1); bnr++) {
+ pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
+ if (!pbus)
+ continue;
+
+ /* Find RCiEP devices on the given bus */
+ if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
+ return;
+ }
+}
+
#ifdef CONFIG_PM
typedef int (*pcie_pm_callback_t)(struct pcie_device *);
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC
2020-07-24 17:22 ` [RFC PATCH 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC Sean V Kelley
@ 2020-07-27 10:49 ` Jonathan Cameron
2020-07-27 15:21 ` Sean V Kelley
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 10:49 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Fri, 24 Jul 2020 10:22:17 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>
> When an RCEC device signals error(s) to a CPU core, the CPU core
> needs to walk all the RCiEPs associated with that RCEC to check
> errors. So add the function pcie_walk_rcec() to walk all RCiEPs
> associated with the RCEC device.
>
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
A few trivial points inline. With those tidied up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/pci/pcie/portdrv.h | 2 +
> drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++++
> 2 files changed, 84 insertions(+)
>
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index af7cf237432a..c11d5ecbad76 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct pcie_port_service_driver *new);
>
> extern struct bus_type pcie_port_bus_type;
> int pcie_port_device_register(struct pci_dev *dev);
> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> + void *userdata);
> #ifdef CONFIG_PM
> int pcie_port_device_suspend(struct device *dev);
> int pcie_port_device_resume_noirq(struct device *dev);
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522ab07d..bdcbb34764c2 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -14,6 +14,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/string.h>
> #include <linux/slab.h>
> +#include <linux/bitops.h>
> #include <linux/aer.h>
>
> #include "../pci.h"
> @@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev *dev)
> return status;
> }
>
> +static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int (*cb)(struct pci_dev *, void *),
> + void *userdata, unsigned long bitmap)
> +{
> + unsigned int dev, fn;
> + struct pci_dev *pdev;
> + int retval;
> +
> + for_each_set_bit(dev, &bitmap, 32) {
> + for (fn = 0; fn < 8; fn++) {
> + pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
> +
> + if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
> + continue;
> +
> + retval = cb(pdev, userdata);
> + if (retval)
> + return retval;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/** pcie_walk_rcec - Walk RCiEP devices associating with RCEC and call callback.
/**
* pcie...
> + * @rcec RCEC whose RCiEP devices should be walked.
> + * @cb Callback to be called for each RCiEP device found.
> + * @userdata Arbitrary pointer to be passed to callback.
> + *
> + * Walk the given RCEC. Call the provided callback on each RCiEP device found.
> + *
> + * We check the return of @cb each time. If it returns anything
> + * other than 0, we break out.
> + *
> + */
> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> + void *userdata)
> +{
> + u32 pos, bitmap, hdr, busn;
> + u8 ver, nextbusn, lastbusn;
> + struct pci_bus *pbus;
> + unsigned int bnr;
> +
> + pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
> + if (!pos)
> + return;
> +
> + pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
> + if (!pbus)
> + return;
> +
> + pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
> +
> + /* Find RCiEP devices on the same bus as the RCEC */
> + if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned long)bitmap))
> + return;
> +
> + /* Check whether RCEC BUSN register is present */
> + pci_read_config_dword(rcec, pos, &hdr);
> + ver = PCI_EXT_CAP_VER(hdr);
> + if (ver < PCI_RCEC_BUSN_REG_VER)
> + return;
> +
> + pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
> + nextbusn = PCI_RCEC_BUSN_NEXT(busn);
> + lastbusn = PCI_RCEC_BUSN_LAST(busn);
> +
> + /* All RCiEP devices are on the same bus as the RCEC */
> + if (nextbusn == 0xff && lastbusn == 0x00)
> + return;
> +
> + for (bnr = nextbusn; bnr < (lastbusn + 1); bnr++) {
Why not bnr <= lastbusn? Seems more intuitive way of making it clear the
range is inclusive.
> + pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
> + if (!pbus)
> + continue;
> +
> + /* Find RCiEP devices on the given bus */
> + if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
> + return;
> + }
> +}
> +
> #ifdef CONFIG_PM
> typedef int (*pcie_pm_callback_t)(struct pcie_device *);
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC
2020-07-27 10:49 ` Jonathan Cameron
@ 2020-07-27 15:21 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 15:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, Raj, Ashok, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
Hi,
Comments below:
On 27 Jul 2020, at 3:49, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:17 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> When an RCEC device signals error(s) to a CPU core, the CPU core
>> needs to walk all the RCiEPs associated with that RCEC to check
>> errors. So add the function pcie_walk_rcec() to walk all RCiEPs
>> associated with the RCEC device.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>
> A few trivial points inline. With those tidied up.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> ---
>> drivers/pci/pcie/portdrv.h | 2 +
>> drivers/pci/pcie/portdrv_core.c | 82
>> +++++++++++++++++++++++++++++++++
>> 2 files changed, 84 insertions(+)
>>
>> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
>> index af7cf237432a..c11d5ecbad76 100644
>> --- a/drivers/pci/pcie/portdrv.h
>> +++ b/drivers/pci/pcie/portdrv.h
>> @@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct
>> pcie_port_service_driver *new);
>>
>> extern struct bus_type pcie_port_bus_type;
>> int pcie_port_device_register(struct pci_dev *dev);
>> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev
>> *, void *),
>> + void *userdata);
>> #ifdef CONFIG_PM
>> int pcie_port_device_suspend(struct device *dev);
>> int pcie_port_device_resume_noirq(struct device *dev);
>> diff --git a/drivers/pci/pcie/portdrv_core.c
>> b/drivers/pci/pcie/portdrv_core.c
>> index 50a9522ab07d..bdcbb34764c2 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -14,6 +14,7 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/string.h>
>> #include <linux/slab.h>
>> +#include <linux/bitops.h>
>> #include <linux/aer.h>
>>
>> #include "../pci.h"
>> @@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev
>> *dev)
>> return status;
>> }
>>
>> +static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int
>> (*cb)(struct pci_dev *, void *),
>> + void *userdata, unsigned long bitmap)
>> +{
>> + unsigned int dev, fn;
>> + struct pci_dev *pdev;
>> + int retval;
>> +
>> + for_each_set_bit(dev, &bitmap, 32) {
>> + for (fn = 0; fn < 8; fn++) {
>> + pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
>> +
>> + if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
>> + continue;
>> +
>> + retval = cb(pdev, userdata);
>> + if (retval)
>> + return retval;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/** pcie_walk_rcec - Walk RCiEP devices associating with RCEC and
>> call callback.
>
> /**
> * pcie...
Will correct.
>
>> + * @rcec RCEC whose RCiEP devices should be walked.
>> + * @cb Callback to be called for each RCiEP device found.
>> + * @userdata Arbitrary pointer to be passed to callback.
>> + *
>> + * Walk the given RCEC. Call the provided callback on each RCiEP
>> device found.
>> + *
>> + * We check the return of @cb each time. If it returns anything
>> + * other than 0, we break out.
>> + *
>> + */
>> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev
>> *, void *),
>> + void *userdata)
>> +{
>> + u32 pos, bitmap, hdr, busn;
>> + u8 ver, nextbusn, lastbusn;
>> + struct pci_bus *pbus;
>> + unsigned int bnr;
>> +
>> + pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
>> + if (!pos)
>> + return;
>> +
>> + pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
>> + if (!pbus)
>> + return;
>> +
>> + pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
>> +
>> + /* Find RCiEP devices on the same bus as the RCEC */
>> + if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned
>> long)bitmap))
>> + return;
>> +
>> + /* Check whether RCEC BUSN register is present */
>> + pci_read_config_dword(rcec, pos, &hdr);
>> + ver = PCI_EXT_CAP_VER(hdr);
>> + if (ver < PCI_RCEC_BUSN_REG_VER)
>> + return;
>> +
>> + pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
>> + nextbusn = PCI_RCEC_BUSN_NEXT(busn);
>> + lastbusn = PCI_RCEC_BUSN_LAST(busn);
>> +
>> + /* All RCiEP devices are on the same bus as the RCEC */
>> + if (nextbusn == 0xff && lastbusn == 0x00)
>> + return;
>> +
>> + for (bnr = nextbusn; bnr < (lastbusn + 1); bnr++) {
>
> Why not bnr <= lastbusn? Seems more intuitive way of making it clear
> the
> range is inclusive.
Good suggestion. Will change.
Thanks,
Sean
>
>
>> + pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
>> + if (!pbus)
>> + continue;
>> +
>> + /* Find RCiEP devices on the given bus */
>> + if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
>> + return;
>> + }
>> +}
>> +
>> #ifdef CONFIG_PM
>> typedef int (*pcie_pm_callback_t)(struct pcie_device *);
>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (2 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 11:00 ` Jonathan Cameron
2020-07-27 14:04 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error Sean V Kelley
` (5 subsequent siblings)
9 siblings, 2 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Sean V Kelley
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Currently the kernel does not handle AER errors for Root Complex integrated
End Points (RCiEPs)[0]. These devices sit on a root bus within the Root Complex
(RC). AER handling is performed by a Root Complex Event Collector (RCEC) [1]
which is a effectively a type of RCiEP on the same root bus.
For an RCEC (technically not a Bridge), error messages "received" from
associated RCiEPs must be enabled for "transmission" in order to cause a
System Error via the Root Control register or (when the Advanced Error
Reporting Capability is present) reporting via the Root Error Command
register and logging in the Root Error Status register and Error Source
Identification register.
In addition to the defined OS level handling of the reset flow for the
associated RCiEPs of an RCEC, it is possible to also have a firmware first
model. In that case there is no need to take any actions on the RCEC because
the firmware is responsible for them. This is true where APEI [2] is used
to report the AER errors via a GHES[v2] HEST entry [3] and relevant
AER CPER record [4] and Firmware First handling is in use.
We effectively end up with two different types of discovery for
purposes of handling AER errors:
1) Normal bus walk - we pass the downstream port above a bus to which
the device is attached and it walks everything below that point.
2) An RCiEP with no visible association with an RCEC as there is no need to
walk devices. In that case, the flow is to just call the callbacks for the actual
device.
A new walk function, similar to pci_bus_walk is provided that takes a pci_dev
instead of a bus. If that dev corresponds to a downstream port it will walk
the subordinate bus of that downstream port. If the dev does not then it
will call the function on that device alone.
[0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex Integrated
Endpoint Rules.
[1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling and Logging
[2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface (APEI)
[3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
[4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
Changes since v2 [1]:
- Renamed to pci_walk_dev_affected() to reflect the aer affected devices
- Localized to err.c and made static
- Added check for RCEC to reflect
- Tightened up commit log from earlier inquiry focused RFC
[1] https://lore.kernel.org/linux-pci/20200622114402.892798-1-Jonathan.Cameron@huawei.com/
---
drivers/pci/pcie/err.c | 55 ++++++++++++++++++++++++++++++++++--------
1 file changed, 45 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 14bb8f54723e..044df004f20b 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -146,38 +146,69 @@ static int report_resume(struct pci_dev *dev, void *data)
return 0;
}
+/** pci_walk_dev_affected - walk devices potentially AER affected
+ * @dev device which may be an RCEC with associated RCiEPs,
+ * an RCiEP associated with an RCEC, 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 port, walk the subordinate bus,
+ * including any bridged devices on buses under this bus.
+ * Call the provided callback on each device found.
+ *
+ * If the device provided has no subordinate bus, call the provided
+ * callback on the device itself.
+ *
+ */
+static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev *, void *),
+ void *userdata)
+{
+ if (dev->subordinate) {
+ pci_walk_bus(dev->subordinate, cb, userdata);
+ } else {
+ cb(dev, userdata);
+ }
+}
+
pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
enum pci_channel_state state,
pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
{
pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
- struct pci_bus *bus;
/*
* Error recovery runs on all subordinates of the first downstream port.
* If the downstream port detected the error, it is cleared at the end.
+ * For RCiEPs we should reset just the RCiEP itself.
*/
if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
- pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
+ pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
+ pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
dev = dev->bus->self;
- bus = dev->subordinate;
pci_dbg(dev, "broadcast error_detected message\n");
if (state == pci_channel_io_frozen) {
- pci_walk_bus(bus, report_frozen_detected, &status);
+ pci_walk_dev_affected(dev, report_frozen_detected, &status);
+ if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
+ pci_warn(dev, "link reset not possible for RCiEP\n");
+ status = PCI_ERS_RESULT_NONE;
+ goto failed;
+ }
+
status = reset_link(dev);
if (status != PCI_ERS_RESULT_RECOVERED) {
pci_warn(dev, "link reset failed\n");
goto failed;
}
} else {
- pci_walk_bus(bus, report_normal_detected, &status);
+ pci_walk_dev_affected(dev, report_normal_detected, &status);
}
if (status == PCI_ERS_RESULT_CAN_RECOVER) {
status = PCI_ERS_RESULT_RECOVERED;
pci_dbg(dev, "broadcast mmio_enabled message\n");
- pci_walk_bus(bus, report_mmio_enabled, &status);
+ pci_walk_dev_affected(dev, report_mmio_enabled, &status);
}
if (status == PCI_ERS_RESULT_NEED_RESET) {
@@ -188,17 +219,21 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
*/
status = PCI_ERS_RESULT_RECOVERED;
pci_dbg(dev, "broadcast slot_reset message\n");
- pci_walk_bus(bus, report_slot_reset, &status);
+ pci_walk_dev_affected(dev, report_slot_reset, &status);
}
if (status != PCI_ERS_RESULT_RECOVERED)
goto failed;
pci_dbg(dev, "broadcast resume message\n");
- pci_walk_bus(bus, report_resume, &status);
+ pci_walk_dev_affected(dev, report_resume, &status);
- pci_aer_clear_device_status(dev);
- pci_aer_clear_nonfatal_status(dev);
+ if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
+ pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
+ pci_aer_clear_device_status(dev);
+ pci_aer_clear_nonfatal_status(dev);
+ }
pci_info(dev, "device recovery successful\n");
return status;
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs
2020-07-24 17:22 ` [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs Sean V Kelley
@ 2020-07-27 11:00 ` Jonathan Cameron
2020-07-27 14:58 ` Sean V Kelley
2020-07-27 14:04 ` Jonathan Cameron
1 sibling, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 11:00 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
On Fri, 24 Jul 2020 10:22:18 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Currently the kernel does not handle AER errors for Root Complex integrated
> End Points (RCiEPs)[0]. These devices sit on a root bus within the Root Complex
> (RC). AER handling is performed by a Root Complex Event Collector (RCEC) [1]
> which is a effectively a type of RCiEP on the same root bus.
>
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
>
> In addition to the defined OS level handling of the reset flow for the
> associated RCiEPs of an RCEC, it is possible to also have a firmware first
> model. In that case there is no need to take any actions on the RCEC because
> the firmware is responsible for them. This is true where APEI [2] is used
> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
> AER CPER record [4] and Firmware First handling is in use.
>
> We effectively end up with two different types of discovery for
> purposes of handling AER errors:
>
> 1) Normal bus walk - we pass the downstream port above a bus to which
> the device is attached and it walks everything below that point.
>
> 2) An RCiEP with no visible association with an RCEC as there is no need to
> walk devices. In that case, the flow is to just call the callbacks for the actual
> device.
>
> A new walk function, similar to pci_bus_walk is provided that takes a pci_dev
> instead of a bus. If that dev corresponds to a downstream port it will walk
> the subordinate bus of that downstream port. If the dev does not then it
> will call the function on that device alone.
>
> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex Integrated
> Endpoint Rules.
> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling and Logging
> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface (APEI)
> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
> Changes since v2 [1]:
>
> - Renamed to pci_walk_dev_affected() to reflect the aer affected devices
Make sense.
> - Localized to err.c and made static
Makes sense.
> - Added check for RCEC to reflect
That comment probably needs a bit more...
> - Tightened up commit log from earlier inquiry focused RFC
Cool.
Looks good to me and I like the new naming.
A few really trivial tidy ups suggested for things that were less than neat in my patch.
Jonathan
>
> [1] https://lore.kernel.org/linux-pci/20200622114402.892798-1-Jonathan.Cameron@huawei.com/
> ---
> drivers/pci/pcie/err.c | 55 ++++++++++++++++++++++++++++++++++--------
> 1 file changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 14bb8f54723e..044df004f20b 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -146,38 +146,69 @@ static int report_resume(struct pci_dev *dev, void *data)
> return 0;
> }
>
> +/** pci_walk_dev_affected - walk devices potentially AER affected
/**
* pci_walk_dev_affected
There is a bit of a mixture in pci files between the two styles, but
I'm fairly sure kernel-doc is supposed to be as I'm suggesting
(I had this wrong due to cut and paste in earlier version!)
> + * @dev device which may be an RCEC with associated RCiEPs,
> + * an RCiEP associated with an RCEC, 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 port, walk the subordinate bus,
> + * including any bridged devices on buses under this bus.
> + * Call the provided callback on each device found.
> + *
> + * If the device provided has no subordinate bus, call the provided
> + * callback on the device itself.
> + *
I also had an ugly pointless newline here. oops :)
> + */
> +static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev *, void *),
> + void *userdata)
> +{
> + if (dev->subordinate) {
> + pci_walk_bus(dev->subordinate, cb, userdata);
> + } else {
> + cb(dev, userdata);
> + }
> +}
> +
> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> enum pci_channel_state state,
> pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
> {
> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> - struct pci_bus *bus;
>
> /*
> * Error recovery runs on all subordinates of the first downstream port.
> * If the downstream port detected the error, it is cleared at the end.
> + * For RCiEPs we should reset just the RCiEP itself.
> */
> if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> - pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
> dev = dev->bus->self;
> - bus = dev->subordinate;
>
> pci_dbg(dev, "broadcast error_detected message\n");
> if (state == pci_channel_io_frozen) {
> - pci_walk_bus(bus, report_frozen_detected, &status);
> + pci_walk_dev_affected(dev, report_frozen_detected, &status);
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> + pci_warn(dev, "link reset not possible for RCiEP\n");
> + status = PCI_ERS_RESULT_NONE;
> + goto failed;
> + }
> +
> status = reset_link(dev);
> if (status != PCI_ERS_RESULT_RECOVERED) {
> pci_warn(dev, "link reset failed\n");
> goto failed;
> }
> } else {
> - pci_walk_bus(bus, report_normal_detected, &status);
> + pci_walk_dev_affected(dev, report_normal_detected, &status);
> }
>
> if (status == PCI_ERS_RESULT_CAN_RECOVER) {
> status = PCI_ERS_RESULT_RECOVERED;
> pci_dbg(dev, "broadcast mmio_enabled message\n");
> - pci_walk_bus(bus, report_mmio_enabled, &status);
> + pci_walk_dev_affected(dev, report_mmio_enabled, &status);
> }
>
> if (status == PCI_ERS_RESULT_NEED_RESET) {
> @@ -188,17 +219,21 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> */
> status = PCI_ERS_RESULT_RECOVERED;
> pci_dbg(dev, "broadcast slot_reset message\n");
> - pci_walk_bus(bus, report_slot_reset, &status);
> + pci_walk_dev_affected(dev, report_slot_reset, &status);
> }
>
> if (status != PCI_ERS_RESULT_RECOVERED)
> goto failed;
>
> pci_dbg(dev, "broadcast resume message\n");
> - pci_walk_bus(bus, report_resume, &status);
> + pci_walk_dev_affected(dev, report_resume, &status);
>
> - pci_aer_clear_device_status(dev);
> - pci_aer_clear_nonfatal_status(dev);
> + if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
> + pci_aer_clear_device_status(dev);
> + pci_aer_clear_nonfatal_status(dev);
> + }
> pci_info(dev, "device recovery successful\n");
> return status;
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs
2020-07-27 11:00 ` Jonathan Cameron
@ 2020-07-27 14:58 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 14:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
On 27 Jul 2020, at 4:00, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:18 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>> Currently the kernel does not handle AER errors for Root Complex
>> integrated
>> End Points (RCiEPs)[0]. These devices sit on a root bus within the
>> Root Complex
>> (RC). AER handling is performed by a Root Complex Event Collector
>> (RCEC) [1]
>> which is a effectively a type of RCiEP on the same root bus.
>>
>> For an RCEC (technically not a Bridge), error messages "received"
>> from
>> associated RCiEPs must be enabled for "transmission" in order to
>> cause a
>> System Error via the Root Control register or (when the Advanced
>> Error
>> Reporting Capability is present) reporting via the Root Error Command
>> register and logging in the Root Error Status register and Error
>> Source
>> Identification register.
>>
>> In addition to the defined OS level handling of the reset flow for
>> the
>> associated RCiEPs of an RCEC, it is possible to also have a firmware
>> first
>> model. In that case there is no need to take any actions on the RCEC
>> because
>> the firmware is responsible for them. This is true where APEI [2] is
>> used
>> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
>> AER CPER record [4] and Firmware First handling is in use.
>>
>> We effectively end up with two different types of discovery for
>> purposes of handling AER errors:
>>
>> 1) Normal bus walk - we pass the downstream port above a bus to which
>> the device is attached and it walks everything below that point.
>>
>> 2) An RCiEP with no visible association with an RCEC as there is no
>> need to
>> walk devices. In that case, the flow is to just call the callbacks
>> for the actual
>> device.
>>
>> A new walk function, similar to pci_bus_walk is provided that takes a
>> pci_dev
>> instead of a bus. If that dev corresponds to a downstream port it
>> will walk
>> the subordinate bus of that downstream port. If the dev does not then
>> it
>> will call the function on that device alone.
>>
>> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex
>> Integrated
>> Endpoint Rules.
>> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling
>> and Logging
>> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface
>> (APEI)
>> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
>> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
>>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> ---
>> Changes since v2 [1]:
>>
>> - Renamed to pci_walk_dev_affected() to reflect the aer affected
>> devices
> Make sense.
>
>> - Localized to err.c and made static
>
> Makes sense.
>
>> - Added check for RCEC to reflect
> That comment probably needs a bit more...
Will add to the details.
>
>> - Tightened up commit log from earlier inquiry focused RFC
> Cool.
>
>
> Looks good to me and I like the new naming.
>
> A few really trivial tidy ups suggested for things that were less than
> neat in my patch.
>
> Jonathan
>
>>
>> [1]
>> https://lore.kernel.org/linux-pci/20200622114402.892798-1-Jonathan.Cameron@huawei.com/
>> ---
>> drivers/pci/pcie/err.c | 55
>> ++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 14bb8f54723e..044df004f20b 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -146,38 +146,69 @@ static int report_resume(struct pci_dev *dev,
>> void *data)
>> return 0;
>> }
>>
>> +/** pci_walk_dev_affected - walk devices potentially AER affected
> /**
> * pci_walk_dev_affected
>
> There is a bit of a mixture in pci files between the two styles, but
> I'm fairly sure kernel-doc is supposed to be as I'm suggesting
> (I had this wrong due to cut and paste in earlier version!)
Will fix.
>
>> + * @dev device which may be an RCEC with associated RCiEPs,
>> + * an RCiEP associated with an RCEC, 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 port, walk the subordinate bus,
>> + * including any bridged devices on buses under this bus.
>> + * Call the provided callback on each device found.
>> + *
>> + * If the device provided has no subordinate bus, call the provided
>> + * callback on the device itself.
>> + *
>
> I also had an ugly pointless newline here. oops :)
Will fix.
Thanks,
Sean
>
>> + */
>> +static void pci_walk_dev_affected(struct pci_dev *dev, int
>> (*cb)(struct pci_dev *, void *),
>> + void *userdata)
>> +{
>> + if (dev->subordinate) {
>> + pci_walk_bus(dev->subordinate, cb, userdata);
>> + } else {
>> + cb(dev, userdata);
>> + }
>> +}
>> +
>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> enum pci_channel_state state,
>> pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
>> {
>> pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
>> - struct pci_bus *bus;
>>
>> /*
>> * Error recovery runs on all subordinates of the first downstream
>> port.
>> * If the downstream port detected the error, it is cleared at the
>> end.
>> + * For RCiEPs we should reset just the RCiEP itself.
>> */
>> if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> - pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
>> dev = dev->bus->self;
>> - bus = dev->subordinate;
>>
>> pci_dbg(dev, "broadcast error_detected message\n");
>> if (state == pci_channel_io_frozen) {
>> - pci_walk_bus(bus, report_frozen_detected, &status);
>> + pci_walk_dev_affected(dev, report_frozen_detected, &status);
>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>> + pci_warn(dev, "link reset not possible for RCiEP\n");
>> + status = PCI_ERS_RESULT_NONE;
>> + goto failed;
>> + }
>> +
>> status = reset_link(dev);
>> if (status != PCI_ERS_RESULT_RECOVERED) {
>> pci_warn(dev, "link reset failed\n");
>> goto failed;
>> }
>> } else {
>> - pci_walk_bus(bus, report_normal_detected, &status);
>> + pci_walk_dev_affected(dev, report_normal_detected, &status);
>> }
>>
>> if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>> status = PCI_ERS_RESULT_RECOVERED;
>> pci_dbg(dev, "broadcast mmio_enabled message\n");
>> - pci_walk_bus(bus, report_mmio_enabled, &status);
>> + pci_walk_dev_affected(dev, report_mmio_enabled, &status);
>> }
>>
>> if (status == PCI_ERS_RESULT_NEED_RESET) {
>> @@ -188,17 +219,21 @@ pci_ers_result_t pcie_do_recovery(struct
>> pci_dev *dev,
>> */
>> status = PCI_ERS_RESULT_RECOVERED;
>> pci_dbg(dev, "broadcast slot_reset message\n");
>> - pci_walk_bus(bus, report_slot_reset, &status);
>> + pci_walk_dev_affected(dev, report_slot_reset, &status);
>> }
>>
>> if (status != PCI_ERS_RESULT_RECOVERED)
>> goto failed;
>>
>> pci_dbg(dev, "broadcast resume message\n");
>> - pci_walk_bus(bus, report_resume, &status);
>> + pci_walk_dev_affected(dev, report_resume, &status);
>>
>> - pci_aer_clear_device_status(dev);
>> - pci_aer_clear_nonfatal_status(dev);
>> + if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
>> + pci_aer_clear_device_status(dev);
>> + pci_aer_clear_nonfatal_status(dev);
>> + }
>> pci_info(dev, "device recovery successful\n");
>> return status;
>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs
2020-07-24 17:22 ` [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs Sean V Kelley
2020-07-27 11:00 ` Jonathan Cameron
@ 2020-07-27 14:04 ` Jonathan Cameron
2020-07-27 15:00 ` Sean V Kelley
1 sibling, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 14:04 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
On Fri, 24 Jul 2020 10:22:18 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Currently the kernel does not handle AER errors for Root Complex integrated
> End Points (RCiEPs)[0]. These devices sit on a root bus within the Root Complex
> (RC). AER handling is performed by a Root Complex Event Collector (RCEC) [1]
> which is a effectively a type of RCiEP on the same root bus.
>
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
>
> In addition to the defined OS level handling of the reset flow for the
> associated RCiEPs of an RCEC, it is possible to also have a firmware first
> model. In that case there is no need to take any actions on the RCEC because
> the firmware is responsible for them. This is true where APEI [2] is used
> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
> AER CPER record [4] and Firmware First handling is in use.
>
> We effectively end up with two different types of discovery for
> purposes of handling AER errors:
>
> 1) Normal bus walk - we pass the downstream port above a bus to which
> the device is attached and it walks everything below that point.
>
> 2) An RCiEP with no visible association with an RCEC as there is no need to
> walk devices. In that case, the flow is to just call the callbacks for the actual
> device.
>
> A new walk function, similar to pci_bus_walk is provided that takes a pci_dev
> instead of a bus. If that dev corresponds to a downstream port it will walk
> the subordinate bus of that downstream port. If the dev does not then it
> will call the function on that device alone.
>
> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex Integrated
> Endpoint Rules.
> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling and Logging
> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface (APEI)
> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
...
> pci_dbg(dev, "broadcast resume message\n");
> - pci_walk_bus(bus, report_resume, &status);
> + pci_walk_dev_affected(dev, report_resume, &status);
>
> - pci_aer_clear_device_status(dev);
> - pci_aer_clear_nonfatal_status(dev);
This code had changed a little in Bjorn's pci/next branch so do a rebase on that
before v2.
> + if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
> + pci_aer_clear_device_status(dev);
> + pci_aer_clear_nonfatal_status(dev);
> + }
> pci_info(dev, "device recovery successful\n");
> return status;
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs
2020-07-27 14:04 ` Jonathan Cameron
@ 2020-07-27 15:00 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 15:00 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, tony.luck, Raj, Ashok, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
On 27 Jul 2020, at 7:04, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:18 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>> Currently the kernel does not handle AER errors for Root Complex
>> integrated
>> End Points (RCiEPs)[0]. These devices sit on a root bus within the
>> Root Complex
>> (RC). AER handling is performed by a Root Complex Event Collector
>> (RCEC) [1]
>> which is a effectively a type of RCiEP on the same root bus.
>>
>> For an RCEC (technically not a Bridge), error messages "received"
>> from
>> associated RCiEPs must be enabled for "transmission" in order to
>> cause a
>> System Error via the Root Control register or (when the Advanced
>> Error
>> Reporting Capability is present) reporting via the Root Error Command
>> register and logging in the Root Error Status register and Error
>> Source
>> Identification register.
>>
>> In addition to the defined OS level handling of the reset flow for
>> the
>> associated RCiEPs of an RCEC, it is possible to also have a firmware
>> first
>> model. In that case there is no need to take any actions on the RCEC
>> because
>> the firmware is responsible for them. This is true where APEI [2] is
>> used
>> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
>> AER CPER record [4] and Firmware First handling is in use.
>>
>> We effectively end up with two different types of discovery for
>> purposes of handling AER errors:
>>
>> 1) Normal bus walk - we pass the downstream port above a bus to which
>> the device is attached and it walks everything below that point.
>>
>> 2) An RCiEP with no visible association with an RCEC as there is no
>> need to
>> walk devices. In that case, the flow is to just call the callbacks
>> for the actual
>> device.
>>
>> A new walk function, similar to pci_bus_walk is provided that takes a
>> pci_dev
>> instead of a bus. If that dev corresponds to a downstream port it
>> will walk
>> the subordinate bus of that downstream port. If the dev does not then
>> it
>> will call the function on that device alone.
>>
>> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex
>> Integrated
>> Endpoint Rules.
>> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling
>> and Logging
>> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface
>> (APEI)
>> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
>> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
>>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> ---
> ...
>
>
>> pci_dbg(dev, "broadcast resume message\n");
>> - pci_walk_bus(bus, report_resume, &status);
>> + pci_walk_dev_affected(dev, report_resume, &status);
>>
>> - pci_aer_clear_device_status(dev);
>> - pci_aer_clear_nonfatal_status(dev);
>
> This code had changed a little in Bjorn's pci/next branch so do a
> rebase on that
> before v2.
Will ensure rebase includes pci/next.
Thanks,
Sean
>
>> + if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
>> + pci_aer_clear_device_status(dev);
>> + pci_aer_clear_nonfatal_status(dev);
>> + }
>> pci_info(dev, "device recovery successful\n");
>> return status;
>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (3 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 4/9] PCI/AER: Extend AER error handling to RCECs Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 11:17 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs Sean V Kelley
` (4 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Attempt to do function level reset for an RCiEP associated with an
RCEC device on fatal error.
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 044df004f20b..9b3ec94bdf1d 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev
}
}
+static enum pci_channel_state flr_on_rciep(struct pci_dev *dev)
+{
+ if (!pcie_has_flr(dev))
+ return PCI_ERS_RESULT_NONE;
+
+ if (pcie_flr(dev))
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
enum pci_channel_state state,
pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
@@ -191,15 +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
if (state == pci_channel_io_frozen) {
pci_walk_dev_affected(dev, report_frozen_detected, &status);
if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
- pci_warn(dev, "link reset not possible for RCiEP\n");
- status = PCI_ERS_RESULT_NONE;
- goto failed;
- }
-
- status = reset_link(dev);
- if (status != PCI_ERS_RESULT_RECOVERED) {
- pci_warn(dev, "link reset failed\n");
- goto failed;
+ status = flr_on_rciep(dev);
+ if (status != PCI_ERS_RESULT_RECOVERED) {
+ pci_warn(dev, "function level reset failed\n");
+ goto failed;
+ }
+ } else {
+ status = reset_link(dev);
+ if (status != PCI_ERS_RESULT_RECOVERED) {
+ pci_warn(dev, "link reset failed\n");
+ goto failed;
+ }
}
} else {
pci_walk_dev_affected(dev, report_normal_detected, &status);
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error
2020-07-24 17:22 ` [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error Sean V Kelley
@ 2020-07-27 11:17 ` Jonathan Cameron
2020-07-28 13:27 ` Zhuo, Qiuxu
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 11:17 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Fri, 24 Jul 2020 10:22:19 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>
> Attempt to do function level reset for an RCiEP associated with an
> RCEC device on fatal error.
I'd like to understand more on your reasoning for flr here.
Is it simply that it is all we can do, or is there some basis
in a spec somewhere?
>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 044df004f20b..9b3ec94bdf1d 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev
> }
> }
>
> +static enum pci_channel_state flr_on_rciep(struct pci_dev *dev)
> +{
> + if (!pcie_has_flr(dev))
> + return PCI_ERS_RESULT_NONE;
> +
> + if (pcie_flr(dev))
> + return PCI_ERS_RESULT_DISCONNECT;
> +
> + return PCI_ERS_RESULT_RECOVERED;
> +}
> +
> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> enum pci_channel_state state,
> pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
> @@ -191,15 +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> if (state == pci_channel_io_frozen) {
> pci_walk_dev_affected(dev, report_frozen_detected, &status);
> if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> - pci_warn(dev, "link reset not possible for RCiEP\n");
> - status = PCI_ERS_RESULT_NONE;
> - goto failed;
> - }
> -
> - status = reset_link(dev);
> - if (status != PCI_ERS_RESULT_RECOVERED) {
> - pci_warn(dev, "link reset failed\n");
> - goto failed;
> + status = flr_on_rciep(dev);
> + if (status != PCI_ERS_RESULT_RECOVERED) {
> + pci_warn(dev, "function level reset failed\n");
> + goto failed;
> + }
> + } else {
> + status = reset_link(dev);
> + if (status != PCI_ERS_RESULT_RECOVERED) {
> + pci_warn(dev, "link reset failed\n");
> + goto failed;
> + }
> }
> } else {
> pci_walk_dev_affected(dev, report_normal_detected, &status);
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error
2020-07-27 11:17 ` Jonathan Cameron
@ 2020-07-28 13:27 ` Zhuo, Qiuxu
2020-07-28 16:14 ` Sean V Kelley
0 siblings, 1 reply; 37+ messages in thread
From: Zhuo, Qiuxu @ 2020-07-28 13:27 UTC (permalink / raw)
To: 'Jonathan Cameron', Kelley, Sean V
Cc: bhelgaas, rjw, ashok.raj, Luck, Tony, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Monday, July 27, 2020 7:17 PM
> To: Kelley, Sean V <sean.v.kelley@intel.com>
> Cc: bhelgaas@google.com; rjw@rjwysocki.net; ashok.raj@kernel.org; Luck,
> Tony <tony.luck@intel.com>;
> sathyanarayanan.kuppuswamy@linux.intel.com; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org; Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> Subject: Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP
> on fatal error
>
> On Fri, 24 Jul 2020 10:22:19 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >
> > Attempt to do function level reset for an RCiEP associated with an
> > RCEC device on fatal error.
>
> I'd like to understand more on your reasoning for flr here.
> Is it simply that it is all we can do, or is there some basis in a spec
> somewhere?
>
Yes. Though there isn't the link reset for the RCiEP here, I think we should still be able to reset the RCiEP via FLR on fatal error, if the RCiEP supports FLR.
-Qiuxu
> >
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > ---
> > drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
> > 1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index
> > 044df004f20b..9b3ec94bdf1d 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct
> pci_dev *dev, int (*cb)(struct pci_dev
> > }
> > }
> >
> > +static enum pci_channel_state flr_on_rciep(struct pci_dev *dev) {
> > + if (!pcie_has_flr(dev))
> > + return PCI_ERS_RESULT_NONE;
> > +
> > + if (pcie_flr(dev))
> > + return PCI_ERS_RESULT_DISCONNECT;
> > +
> > + return PCI_ERS_RESULT_RECOVERED;
> > +}
> > +
> > pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > enum pci_channel_state state,
> > pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
> @@ -191,15
> > +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > if (state == pci_channel_io_frozen) {
> > pci_walk_dev_affected(dev, report_frozen_detected,
> &status);
> > if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> > - pci_warn(dev, "link reset not possible for RCiEP\n");
> > - status = PCI_ERS_RESULT_NONE;
> > - goto failed;
> > - }
> > -
> > - status = reset_link(dev);
> > - if (status != PCI_ERS_RESULT_RECOVERED) {
> > - pci_warn(dev, "link reset failed\n");
> > - goto failed;
> > + status = flr_on_rciep(dev);
> > + if (status != PCI_ERS_RESULT_RECOVERED) {
> > + pci_warn(dev, "function level reset failed\n");
> > + goto failed;
> > + }
> > + } else {
> > + status = reset_link(dev);
> > + if (status != PCI_ERS_RESULT_RECOVERED) {
> > + pci_warn(dev, "link reset failed\n");
> > + goto failed;
> > + }
> > }
> > } else {
> > pci_walk_dev_affected(dev, report_normal_detected,
> &status);
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error
2020-07-28 13:27 ` Zhuo, Qiuxu
@ 2020-07-28 16:14 ` Sean V Kelley
2020-07-28 17:02 ` Jonathan Cameron
0 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-28 16:14 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, Zhuo, Qiuxu, Luck, Tony,
sathyanarayanan.kuppuswamy, linux-pci, linux-kernel, Raj, Ashok
On 28 Jul 2020, at 6:27, Zhuo, Qiuxu wrote:
>> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> Sent: Monday, July 27, 2020 7:17 PM
>> To: Kelley, Sean V <sean.v.kelley@intel.com>
>> Cc: bhelgaas@google.com; rjw@rjwysocki.net; ashok.raj@kernel.org;
>> Luck,
>> Tony <tony.luck@intel.com>;
>> sathyanarayanan.kuppuswamy@linux.intel.com;
>> linux-pci@vger.kernel.org;
>> linux-kernel@vger.kernel.org; Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
>> Subject: Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to
>> RCiEP
>> on fatal error
>>
>> On Fri, 24 Jul 2020 10:22:19 -0700
>> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>>
>>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>>
>>> Attempt to do function level reset for an RCiEP associated with an
>>> RCEC device on fatal error.
>>
>> I'd like to understand more on your reasoning for flr here.
>> Is it simply that it is all we can do, or is there some basis in a
>> spec
>> somewhere?
>>
>
> Yes. Though there isn't the link reset for the RCiEP here, I think we
> should still be able to reset the RCiEP via FLR on fatal error, if the
> RCiEP supports FLR.
>
> -Qiuxu
>
Also see PCIe 5.0-1, Sec. 6.6.2 Function Level Reset (FLR)
Implementation of FLR is optional (not required), but is strongly
recommended. For an example use case consider CXL. Function 0 DVSEC
instances control for the CXL functionality of the entire CXL device.
FLR may succeed in recovering from CXL.io domain errors.
Thanks,
Sean
>>>
>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>> ---
>>> drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index
>>> 044df004f20b..9b3ec94bdf1d 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct
>> pci_dev *dev, int (*cb)(struct pci_dev
>>> }
>>> }
>>>
>>> +static enum pci_channel_state flr_on_rciep(struct pci_dev *dev) {
>>> +if (!pcie_has_flr(dev))
>>> +return PCI_ERS_RESULT_NONE;
>>> +
>>> +if (pcie_flr(dev))
>>> +return PCI_ERS_RESULT_DISCONNECT;
>>> +
>>> +return PCI_ERS_RESULT_RECOVERED;
>>> +}
>>> +
>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>> enum pci_channel_state state,
>>> pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
>> @@ -191,15
>>> +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>> if (state == pci_channel_io_frozen) {
>>> pci_walk_dev_affected(dev, report_frozen_detected,
>> &status);
>>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>>> -pci_warn(dev, "link reset not possible for RCiEP\n");
>>> -status = PCI_ERS_RESULT_NONE;
>>> -goto failed;
>>> -}
>>> -
>>> -status = reset_link(dev);
>>> -if (status != PCI_ERS_RESULT_RECOVERED) {
>>> -pci_warn(dev, "link reset failed\n");
>>> -goto failed;
>>> +status = flr_on_rciep(dev);
>>> +if (status != PCI_ERS_RESULT_RECOVERED) {
>>> +pci_warn(dev, "function level reset failed\n");
>>> +goto failed;
>>> +}
>>> +} else {
>>> +status = reset_link(dev);
>>> +if (status != PCI_ERS_RESULT_RECOVERED) {
>>> +pci_warn(dev, "link reset failed\n");
>>> +goto failed;
>>> +}
>>> }
>>> } else {
>>> pci_walk_dev_affected(dev, report_normal_detected,
>> &status);
>>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error
2020-07-28 16:14 ` Sean V Kelley
@ 2020-07-28 17:02 ` Jonathan Cameron
2020-07-28 17:42 ` Sean V Kelley
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-28 17:02 UTC (permalink / raw)
To: Sean V Kelley, bhelgaas, Zhuo, Qiuxu
Cc: rjw, Luck, Tony, sathyanarayanan.kuppuswamy, linux-pci,
linux-kernel, Raj, Ashok
On Tue, 28 Jul 2020 09:14:11 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> On 28 Jul 2020, at 6:27, Zhuo, Qiuxu wrote:
>
> >> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> >> Sent: Monday, July 27, 2020 7:17 PM
> >> To: Kelley, Sean V <sean.v.kelley@intel.com>
> >> Cc: bhelgaas@google.com; rjw@rjwysocki.net; ashok.raj@kernel.org;
> >> Luck,
> >> Tony <tony.luck@intel.com>;
> >> sathyanarayanan.kuppuswamy@linux.intel.com;
> >> linux-pci@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> >> Subject: Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to
> >> RCiEP
> >> on fatal error
> >>
> >> On Fri, 24 Jul 2020 10:22:19 -0700
> >> Sean V Kelley <sean.v.kelley@intel.com> wrote:
> >>
> >>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >>>
> >>> Attempt to do function level reset for an RCiEP associated with an
> >>> RCEC device on fatal error.
> >>
> >> I'd like to understand more on your reasoning for flr here.
> >> Is it simply that it is all we can do, or is there some basis in a
> >> spec
> >> somewhere?
> >>
> >
> > Yes. Though there isn't the link reset for the RCiEP here, I think we
> > should still be able to reset the RCiEP via FLR on fatal error, if the
> > RCiEP supports FLR.
> >
> > -Qiuxu
> >
>
> Also see PCIe 5.0-1, Sec. 6.6.2 Function Level Reset (FLR)
>
> Implementation of FLR is optional (not required), but is strongly
> recommended. For an example use case consider CXL. Function 0 DVSEC
> instances control for the CXL functionality of the entire CXL device.
> FLR may succeed in recovering from CXL.io domain errors.
That feels a little bit of a weak argument in favour. PCI spec lists examples
of use only for FLR and I can't see this matching any of them, but then they
are only examples, so we could argue it doesn't exclude this use. It's not
allowed to affect the link state, but I guess it 'might' recover from some
other type of error?
I'd have read the statement in the CXL spec you are referring to as matching
with the first example in the PCIe spec which is about recovering from
software errors. For example, unexpected VM tear down.
@Bjorn / All. What's your view on using FLR as a reset to do when you don't
have any other hammers to use?
Personally I don't have a particular problem with this, it just doesn't fit
with my mental model of what FLR is for (which may well need adjusting :)
Jonathan
>
> Thanks,
>
> Sean
>
> >>>
> >>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >>> ---
> >>> drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
> >>> 1 file changed, 22 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index
> >>> 044df004f20b..9b3ec94bdf1d 100644
> >>> --- a/drivers/pci/pcie/err.c
> >>> +++ b/drivers/pci/pcie/err.c
> >>> @@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct
> >> pci_dev *dev, int (*cb)(struct pci_dev
> >>> }
> >>> }
> >>>
> >>> +static enum pci_channel_state flr_on_rciep(struct pci_dev *dev) {
> >>> +if (!pcie_has_flr(dev))
> >>> +return PCI_ERS_RESULT_NONE;
> >>> +
> >>> +if (pcie_flr(dev))
> >>> +return PCI_ERS_RESULT_DISCONNECT;
> >>> +
> >>> +return PCI_ERS_RESULT_RECOVERED;
> >>> +}
> >>> +
> >>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>> enum pci_channel_state state,
> >>> pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
> >> @@ -191,15
> >>> +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>> if (state == pci_channel_io_frozen) {
> >>> pci_walk_dev_affected(dev, report_frozen_detected,
> >> &status);
> >>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> >>> -pci_warn(dev, "link reset not possible for RCiEP\n");
> >>> -status = PCI_ERS_RESULT_NONE;
> >>> -goto failed;
> >>> -}
> >>> -
> >>> -status = reset_link(dev);
> >>> -if (status != PCI_ERS_RESULT_RECOVERED) {
> >>> -pci_warn(dev, "link reset failed\n");
> >>> -goto failed;
> >>> +status = flr_on_rciep(dev);
> >>> +if (status != PCI_ERS_RESULT_RECOVERED) {
> >>> +pci_warn(dev, "function level reset failed\n");
> >>> +goto failed;
> >>> +}
> >>> +} else {
> >>> +status = reset_link(dev);
> >>> +if (status != PCI_ERS_RESULT_RECOVERED) {
> >>> +pci_warn(dev, "link reset failed\n");
> >>> +goto failed;
> >>> +}
> >>> }
> >>> } else {
> >>> pci_walk_dev_affected(dev, report_normal_detected,
> >> &status);
> >>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error
2020-07-28 17:02 ` Jonathan Cameron
@ 2020-07-28 17:42 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-28 17:42 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, Zhuo, Qiuxu, rjw, Luck, Tony,
sathyanarayanan.kuppuswamy, linux-pci, linux-kernel, Raj, Ashok
On 28 Jul 2020, at 10:02, Jonathan Cameron wrote:
> On Tue, 28 Jul 2020 09:14:11 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> On 28 Jul 2020, at 6:27, Zhuo, Qiuxu wrote:
>>
>>>> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>>>> Sent: Monday, July 27, 2020 7:17 PM
>>>> To: Kelley, Sean V <sean.v.kelley@intel.com>
>>>> Cc: bhelgaas@google.com; rjw@rjwysocki.net; ashok.raj@kernel.org;
>>>> Luck,
>>>> Tony <tony.luck@intel.com>;
>>>> sathyanarayanan.kuppuswamy@linux.intel.com;
>>>> linux-pci@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
>>>> Subject: Re: [RFC PATCH 5/9] PCI/AER: Apply function level reset to
>>>> RCiEP
>>>> on fatal error
>>>>
>>>> On Fri, 24 Jul 2020 10:22:19 -0700
>>>> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>>>>
>>>>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>>>>
>>>>> Attempt to do function level reset for an RCiEP associated with an
>>>>> RCEC device on fatal error.
>>>>
>>>> I'd like to understand more on your reasoning for flr here.
>>>> Is it simply that it is all we can do, or is there some basis in a
>>>> spec
>>>> somewhere?
>>>>
>>>
>>> Yes. Though there isn't the link reset for the RCiEP here, I think
>>> we
>>> should still be able to reset the RCiEP via FLR on fatal error, if
>>> the
>>> RCiEP supports FLR.
>>>
>>> -Qiuxu
>>>
>>
>> Also see PCIe 5.0-1, Sec. 6.6.2 Function Level Reset (FLR)
>>
>> Implementation of FLR is optional (not required), but is strongly
>> recommended. For an example use case consider CXL. Function 0 DVSEC
>> instances control for the CXL functionality of the entire CXL device.
>> FLR may succeed in recovering from CXL.io domain errors.
>
> That feels a little bit of a weak argument in favour. PCI spec lists
> examples
> of use only for FLR and I can't see this matching any of them, but
> then they
> are only examples, so we could argue it doesn't exclude this use. It's
> not
> allowed to affect the link state, but I guess it 'might' recover from
> some
> other type of error?
>
> I'd have read the statement in the CXL spec you are referring to as
> matching
> with the first example in the PCIe spec which is about recovering from
> software errors. For example, unexpected VM tear down.
From my perspective, it can add value as the point is to address device
functions and their associated software states. As the section in the
spec goes on to state:
“The FLR mechanism enables software to quiesce and reset Endpoint
hardware with Function-level granularity. Three example usage models
illustrate the benefits of this feature:…”
Later changes in CXL 2.0 Section 9.8 (as of 0.9 draft) further look to
extend FLR with an eFLR or now referred to as CXL Reset.
“All Functions in a CXL 2.0 (Single Logical Device) SLD that
participate in CXL.cache or CXL.mem are required to support either FLR
or CXL Reset. MLDs (Multiple Logical Devices), on the other hand, are
required to support CXL Reset.”
In my mind the question is whether this change is too limited in scope
with this patch series (RCiEP) and whether FLR should be considered in a
broader, i.e., EP, as a ‘hammer’ so to speak.
Thanks,
Sean
>
> @Bjorn / All. What's your view on using FLR as a reset to do when you
> don't
> have any other hammers to use?
>
> Personally I don't have a particular problem with this, it just
> doesn't fit
> with my mental model of what FLR is for (which may well need adjusting
> :)
>
> Jonathan
>
>
>>
>> Thanks,
>>
>> Sean
>>
>>>>>
>>>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>>>> ---
>>>>> drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
>>>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index
>>>>> 044df004f20b..9b3ec94bdf1d 100644
>>>>> --- a/drivers/pci/pcie/err.c
>>>>> +++ b/drivers/pci/pcie/err.c
>>>>> @@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct
>>>> pci_dev *dev, int (*cb)(struct pci_dev
>>>>> }
>>>>> }
>>>>>
>>>>> +static enum pci_channel_state flr_on_rciep(struct pci_dev *dev) {
>>>>> +if (!pcie_has_flr(dev))
>>>>> +return PCI_ERS_RESULT_NONE;
>>>>> +
>>>>> +if (pcie_flr(dev))
>>>>> +return PCI_ERS_RESULT_DISCONNECT;
>>>>> +
>>>>> +return PCI_ERS_RESULT_RECOVERED;
>>>>> +}
>>>>> +
>>>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>> enum pci_channel_state state,
>>>>> pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
>>>> @@ -191,15
>>>>> +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>> if (state == pci_channel_io_frozen) {
>>>>> pci_walk_dev_affected(dev, report_frozen_detected,
>>>> &status);
>>>>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>>>>> -pci_warn(dev, "link reset not possible for RCiEP\n");
>>>>> -status = PCI_ERS_RESULT_NONE;
>>>>> -goto failed;
>>>>> -}
>>>>> -
>>>>> -status = reset_link(dev);
>>>>> -if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>> -pci_warn(dev, "link reset failed\n");
>>>>> -goto failed;
>>>>> +status = flr_on_rciep(dev);
>>>>> +if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>> +pci_warn(dev, "function level reset failed\n");
>>>>> +goto failed;
>>>>> +}
>>>>> +} else {
>>>>> +status = reset_link(dev);
>>>>> +if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>> +pci_warn(dev, "link reset failed\n");
>>>>> +goto failed;
>>>>> +}
>>>>> }
>>>>> } else {
>>>>> pci_walk_dev_affected(dev, report_normal_detected,
>>>> &status);
>>>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (4 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 11:23 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling Sean V Kelley
` (3 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo, Sean V Kelley
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.
Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
drivers/pci/pcie/aer.c | 9 +++++----
drivers/pci/pcie/err.c | 9 +++++++++
drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
include/linux/pci.h | 3 +++
4 files changed, 32 insertions(+), 4 deletions(-)
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, ®32);
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, ®32);
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 9b3ec94bdf1d..0aae7643132e 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -203,6 +203,11 @@ 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).
+ */
+ reset_link(dev->rcec);
if (status != PCI_ERS_RESULT_RECOVERED) {
pci_warn(dev, "function level reset failed\n");
goto failed;
@@ -246,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) {
+ 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..f9409a0110c2 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_info(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 34c1c4f45288..e920f29df40b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -326,6 +326,9 @@ struct pci_dev {
#ifdef CONFIG_PCIEAER
u16 aer_cap; /* AER capability offset */
struct aer_stats *aer_stats; /* AER stats for this device */
+#endif
+#ifdef CONFIG_PCIEPORTBUS
+ struct pci_dev *rcec; /* Associated RCEC device */
#endif
u8 pcie_cap; /* PCIe capability offset */
u8 msi_cap; /* MSI capability offset */
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs
2020-07-24 17:22 ` [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs Sean V Kelley
@ 2020-07-27 11:23 ` Jonathan Cameron
2020-07-27 15:39 ` Sean V Kelley
2020-07-27 16:11 ` Jonathan Cameron
0 siblings, 2 replies; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 11:23 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Fri, 24 Jul 2020 10:22:20 -0700
Sean V Kelley <sean.v.kelley@intel.com> 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.
>
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
I haven't tested yet, but I think there is one path in here that breaks
my case (no OS visible rcec / all done in firmware GHESv2 / APEI)
Jonathan
> ---
> drivers/pci/pcie/aer.c | 9 +++++----
> drivers/pci/pcie/err.c | 9 +++++++++
> drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
> include/linux/pci.h | 3 +++
> 4 files changed, 32 insertions(+), 4 deletions(-)
>
> 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, ®32);
> 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, ®32);
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 9b3ec94bdf1d..0aae7643132e 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -203,6 +203,11 @@ 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).
> + */
> + reset_link(dev->rcec);
This looks dangerous for my case where APEI / GHESv2 is used. In that case
we don't expose an RCEC at all. I don't think the reset_link callback
is safe to a null pointer here. Fix may be as simple as
if (dev->rcec)
reset_link(dev->rcec);
> if (status != PCI_ERS_RESULT_RECOVERED) {
> pci_warn(dev, "function level reset failed\n");
> goto failed;
> @@ -246,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) {
> + pci_aer_clear_device_status(dev->rcec);
> + pci_aer_clear_nonfatal_status(dev->rcec);
These may be safe as in my both now have protections for !pcie_aer_is_native.
> }
> +
> 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..f9409a0110c2 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_info(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));
We may want to make this debug info at somepoint if we have a way
of discovering it from userspace. The PCI boot up is extremely
verbose already!
> +
> + 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 34c1c4f45288..e920f29df40b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -326,6 +326,9 @@ struct pci_dev {
> #ifdef CONFIG_PCIEAER
> u16 aer_cap; /* AER capability offset */
> struct aer_stats *aer_stats; /* AER stats for this device */
> +#endif
> +#ifdef CONFIG_PCIEPORTBUS
> + struct pci_dev *rcec; /* Associated RCEC device */
> #endif
> u8 pcie_cap; /* PCIe capability offset */
> u8 msi_cap; /* MSI capability offset */
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs
2020-07-27 11:23 ` Jonathan Cameron
@ 2020-07-27 15:39 ` Sean V Kelley
2020-07-27 16:11 ` Jonathan Cameron
1 sibling, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 15:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, Raj, Ashok, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On 27 Jul 2020, at 4:23, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:20 -0700
> Sean V Kelley <sean.v.kelley@intel.com> 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.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>
> I haven't tested yet, but I think there is one path in here that
> breaks
> my case (no OS visible rcec / all done in firmware GHESv2 / APEI)
>
> Jonathan
Okay, keep me up to date and if there is a way I can reproduce or better
test for your use case let me know and I will do so.
>
>> ---
>> drivers/pci/pcie/aer.c | 9 +++++----
>> drivers/pci/pcie/err.c | 9 +++++++++
>> drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
>> include/linux/pci.h | 3 +++
>> 4 files changed, 32 insertions(+), 4 deletions(-)
>>
>> 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, ®32);
>> 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, ®32);
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 9b3ec94bdf1d..0aae7643132e 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -203,6 +203,11 @@ 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).
>> + */
>> + reset_link(dev->rcec);
>
> This looks dangerous for my case where APEI / GHESv2 is used. In that
> case
> we don't expose an RCEC at all. I don't think the reset_link
> callback
> is safe to a null pointer here. Fix may be as simple as
> if (dev->rcec)
> reset_link(dev->rcec);
Good catch. Will look to NULL case as well as walk through code where
dev->rcec is further referenced (below and elsewhere) to ensure nothing
falls through. Glad you are pointing these out.
>
>
>> if (status != PCI_ERS_RESULT_RECOVERED) {
>> pci_warn(dev, "function level reset failed\n");
>> goto failed;
>> @@ -246,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) {
>> + pci_aer_clear_device_status(dev->rcec);
>> + pci_aer_clear_nonfatal_status(dev->rcec);
>
> These may be safe as in my both now have protections for
> !pcie_aer_is_native.
>
I’ll have a look again for the NULL case
>> }
>> +
>> 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..f9409a0110c2 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_info(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));
>
> We may want to make this debug info at somepoint if we have a way
> of discovering it from userspace. The PCI boot up is extremely
> verbose already!
I’ve submitted patches to pciutils, which expose this very detail:
https://lore.kernel.org/linux-pci/20200624223940.240463-1-sean.v.kelley@linux.intel.com/
Flexible either way, could remove or make it debug info.
Thanks,
Sean
>
>> +
>> + 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 34c1c4f45288..e920f29df40b 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -326,6 +326,9 @@ struct pci_dev {
>> #ifdef CONFIG_PCIEAER
>> u16 aer_cap; /* AER capability offset */
>> struct aer_stats *aer_stats; /* AER stats for this device */
>> +#endif
>> +#ifdef CONFIG_PCIEPORTBUS
>> + struct pci_dev *rcec; /* Associated RCEC device */
>> #endif
>> u8 pcie_cap; /* PCIe capability offset */
>> u8 msi_cap; /* MSI capability offset */
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs
2020-07-27 11:23 ` Jonathan Cameron
2020-07-27 15:39 ` Sean V Kelley
@ 2020-07-27 16:11 ` Jonathan Cameron
2020-07-27 16:28 ` Sean V Kelley
1 sibling, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 16:11 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Mon, 27 Jul 2020 12:23:58 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> On Fri, 24 Jul 2020 10:22:20 -0700
> Sean V Kelley <sean.v.kelley@intel.com> 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.
> >
> > Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>
> I haven't tested yet, but I think there is one path in here that breaks
> my case (no OS visible rcec / all done in firmware GHESv2 / APEI)
>
> Jonathan
>
> > ---
> > drivers/pci/pcie/aer.c | 9 +++++----
> > drivers/pci/pcie/err.c | 9 +++++++++
> > drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
> > include/linux/pci.h | 3 +++
> > 4 files changed, 32 insertions(+), 4 deletions(-)
> >
> > 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, ®32);
> > 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, ®32);
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index 9b3ec94bdf1d..0aae7643132e 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -203,6 +203,11 @@ 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).
> > + */
> > + reset_link(dev->rcec);
>
> This looks dangerous for my case where APEI / GHESv2 is used. In that case
> we don't expose an RCEC at all. I don't think the reset_link callback
> is safe to a null pointer here. Fix may be as simple as
> if (dev->rcec)
> reset_link(dev->rcec);
>
>
> > if (status != PCI_ERS_RESULT_RECOVERED) {
> > pci_warn(dev, "function level reset failed\n");
> > goto failed;
> > @@ -246,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) {
> > + pci_aer_clear_device_status(dev->rcec);
This needs updating as well to match current approach in Bjorn's tree
(reworked version of my fix for this in th !is_native case)
> > + pci_aer_clear_nonfatal_status(dev->rcec);
>
> These may be safe as in my both now have protections for !pcie_aer_is_native.
Except I'm wrong. That function performs a local check based
on the pci_dev passed in, so blows up anyway.
So this whole block needs an if(dev->rcec), or potentially protect it with
a call to pci_aer_is_native(dev) on the basis we shouldn't be able to get
here unless we are not doing native aer or we have an rcec.
Jonathan
>
> > }
> > +
> > 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..f9409a0110c2 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_info(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));
>
> We may want to make this debug info at somepoint if we have a way
> of discovering it from userspace. The PCI boot up is extremely
> verbose already!
>
> > +
> > + 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 34c1c4f45288..e920f29df40b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -326,6 +326,9 @@ struct pci_dev {
> > #ifdef CONFIG_PCIEAER
> > u16 aer_cap; /* AER capability offset */
> > struct aer_stats *aer_stats; /* AER stats for this device */
> > +#endif
> > +#ifdef CONFIG_PCIEPORTBUS
> > + struct pci_dev *rcec; /* Associated RCEC device */
> > #endif
> > u8 pcie_cap; /* PCIe capability offset */
> > u8 msi_cap; /* MSI capability offset */
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs
2020-07-27 16:11 ` Jonathan Cameron
@ 2020-07-27 16:28 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 16:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, Raj, Ashok, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On 27 Jul 2020, at 9:11, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 12:23:58 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
>> On Fri, 24 Jul 2020 10:22:20 -0700
>> Sean V Kelley <sean.v.kelley@intel.com> 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.
>>>
>>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>>
>> I haven't tested yet, but I think there is one path in here that
>> breaks
>> my case (no OS visible rcec / all done in firmware GHESv2 / APEI)
>>
>> Jonathan
>>
>>> ---
>>> drivers/pci/pcie/aer.c | 9 +++++----
>>> drivers/pci/pcie/err.c | 9 +++++++++
>>> drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
>>> include/linux/pci.h | 3 +++
>>> 4 files changed, 32 insertions(+), 4 deletions(-)
>>>
>>> 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, ®32);
>>> 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, ®32);
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>> index 9b3ec94bdf1d..0aae7643132e 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -203,6 +203,11 @@ 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).
>>> + */
>>> + reset_link(dev->rcec);
>>
>> This looks dangerous for my case where APEI / GHESv2 is used. In
>> that case
>> we don't expose an RCEC at all. I don't think the reset_link
>> callback
>> is safe to a null pointer here. Fix may be as simple as
>> if (dev->rcec)
>> reset_link(dev->rcec);
>>
>>
>>> if (status != PCI_ERS_RESULT_RECOVERED) {
>>> pci_warn(dev, "function level reset failed\n");
>>> goto failed;
>>> @@ -246,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) {
>>> + pci_aer_clear_device_status(dev->rcec);
> This needs updating as well to match current approach in Bjorn's tree
>
> (reworked version of my fix for this in th !is_native case)
Will rework.
>
>>> + pci_aer_clear_nonfatal_status(dev->rcec);
>>
>> These may be safe as in my both now have protections for
>> !pcie_aer_is_native.
> Except I'm wrong. That function performs a local check based
> on the pci_dev passed in, so blows up anyway.
>
> So this whole block needs an if(dev->rcec), or potentially protect it
> with
> a call to pci_aer_is_native(dev) on the basis we shouldn't be able to
> get
> here unless we are not doing native aer or we have an rcec.
>
> Jonathan
Thanks Jonathan. Will rework that flow and checks on dev->rcec based on
Bjorn’s current tree.
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..f9409a0110c2 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_info(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));
>>
>> We may want to make this debug info at somepoint if we have a way
>> of discovering it from userspace. The PCI boot up is extremely
>> verbose already!
>>
>>> +
>>> + 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 34c1c4f45288..e920f29df40b 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -326,6 +326,9 @@ struct pci_dev {
>>> #ifdef CONFIG_PCIEAER
>>> u16 aer_cap; /* AER capability offset */
>>> struct aer_stats *aer_stats; /* AER stats for this device */
>>> +#endif
>>> +#ifdef CONFIG_PCIEPORTBUS
>>> + struct pci_dev *rcec; /* Associated RCEC device */
>>> #endif
>>> u8 pcie_cap; /* PCIe capability offset */
>>> u8 msi_cap; /* MSI capability offset */
>>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (5 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 12:22 ` Jonathan Cameron
2020-07-24 17:22 ` [RFC PATCH 8/9] PCI/PME: Add RCEC PME handling Sean V Kelley
` (2 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Sean V Kelley, Qiuxu Zhuo
The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
and also have the AER capability. So add RCEC support to the current AER
service driver and attach the AER service driver to the RCEC device.
Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
drivers/pci/pcie/aer.c | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f1bf06be449e..7cc430c74c46 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -303,7 +303,7 @@ int pci_aer_raw_clear_status(struct pci_dev *dev)
return -EIO;
port_type = pci_pcie_type(dev);
- if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
+ if (port_type == PCI_EXP_TYPE_ROOT_PORT || port_type == PCI_EXP_TYPE_RC_EC) {
pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status);
pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status);
}
@@ -389,6 +389,12 @@ void pci_aer_init(struct pci_dev *dev)
pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR, sizeof(u32) * n);
pci_aer_clear_status(dev);
+
+ if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) {
+ if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_RCEC))
+ return;
+ pci_info(dev, "AER: RCEC CAP FOUND and cap_has_rtctl = %d\n", n);
+ }
}
void pci_aer_exit(struct pci_dev *dev)
@@ -577,7 +583,8 @@ static umode_t aer_stats_attrs_are_visible(struct kobject *kobj,
if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
a == &dev_attr_aer_rootport_total_err_fatal.attr ||
a == &dev_attr_aer_rootport_total_err_nonfatal.attr) &&
- pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)
+ ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
+ (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC)))
return 0;
return a->mode;
@@ -894,7 +901,10 @@ static bool find_source_device(struct pci_dev *parent,
if (result)
return true;
- pci_walk_bus(parent->subordinate, find_device_iter, e_info);
+ if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
+ pcie_walk_rcec(parent, find_device_iter, e_info);
+ else
+ pci_walk_bus(parent->subordinate, find_device_iter, e_info);
if (!e_info->error_dev_num) {
pci_info(parent, "can't find device of ID%04x\n", e_info->id);
@@ -1030,6 +1040,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
if (!(info->status & ~info->mask))
return 0;
} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+ pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
info->severity == AER_NONFATAL) {
@@ -1182,6 +1193,8 @@ static int set_device_error_reporting(struct pci_dev *dev, void *data)
int type = pci_pcie_type(dev);
if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
+ (type == PCI_EXP_TYPE_RC_EC) ||
+ (type == PCI_EXP_TYPE_RC_END) ||
(type == PCI_EXP_TYPE_UPSTREAM) ||
(type == PCI_EXP_TYPE_DOWNSTREAM)) {
if (enable)
@@ -1206,9 +1219,11 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
{
set_device_error_reporting(dev, &enable);
- if (!dev->subordinate)
- return;
- pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
+ if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
+ pcie_walk_rcec(dev, set_device_error_reporting, &enable);
+ else if (dev->subordinate)
+ pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
+
}
/**
@@ -1306,6 +1321,11 @@ static int aer_probe(struct pcie_device *dev)
struct device *device = &dev->device;
struct pci_dev *port = dev->port;
+ /* Limit to Root Ports or Root Complex Event Collectors */
+ if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
+ (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
+ return -ENODEV;
+
rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
if (!rpc)
return -ENOMEM;
@@ -1362,7 +1382,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
static struct pcie_port_service_driver aerdriver = {
.name = "aer",
- .port_type = PCI_EXP_TYPE_ROOT_PORT,
+ .port_type = PCIE_ANY_PORT,
.service = PCIE_PORT_SERVICE_AER,
.probe = aer_probe,
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling
2020-07-24 17:22 ` [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling Sean V Kelley
@ 2020-07-27 12:22 ` Jonathan Cameron
2020-07-27 15:19 ` Sean V Kelley
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 12:22 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Fri, 24 Jul 2020 10:22:21 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
> and also have the AER capability. So add RCEC support to the current AER
> service driver and attach the AER service driver to the RCEC device.
>
> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
A few questions and comments for this patch.
See inline.
Jonathan
> ---
> drivers/pci/pcie/aer.c | 34 +++++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f1bf06be449e..7cc430c74c46 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -303,7 +303,7 @@ int pci_aer_raw_clear_status(struct pci_dev *dev)
> return -EIO;
>
> port_type = pci_pcie_type(dev);
> - if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
> + if (port_type == PCI_EXP_TYPE_ROOT_PORT || port_type == PCI_EXP_TYPE_RC_EC) {
> pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status);
> pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status);
> }
> @@ -389,6 +389,12 @@ void pci_aer_init(struct pci_dev *dev)
> pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR, sizeof(u32) * n);
>
> pci_aer_clear_status(dev);
> +
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) {
> + if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_RCEC))
> + return;
> + pci_info(dev, "AER: RCEC CAP FOUND and cap_has_rtctl = %d\n", n);
It feels like failing to find an RC_EC extended cap in a RCEC deserved
a nice strong error message. Perhaps this isn't the right place to do it
though. For that matter, why are we checking for it here?
> + }
> }
>
> void pci_aer_exit(struct pci_dev *dev)
> @@ -577,7 +583,8 @@ static umode_t aer_stats_attrs_are_visible(struct kobject *kobj,
> if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
> a == &dev_attr_aer_rootport_total_err_fatal.attr ||
> a == &dev_attr_aer_rootport_total_err_nonfatal.attr) &&
It is a bit ugly to have these called rootport_total_err etc for the rcec.
Perhaps we should just add additional attributes to reflect we are looking at
an RCEC?
> - pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)
> + ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
> + (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC)))
> return 0;
>
> return a->mode;
> @@ -894,7 +901,10 @@ static bool find_source_device(struct pci_dev *parent,
> if (result)
> return true;
>
> - pci_walk_bus(parent->subordinate, find_device_iter, e_info);
> + if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
> + pcie_walk_rcec(parent, find_device_iter, e_info);
> + else
> + pci_walk_bus(parent->subordinate, find_device_iter, e_info);
>
> if (!e_info->error_dev_num) {
> pci_info(parent, "can't find device of ID%04x\n", e_info->id);
> @@ -1030,6 +1040,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
> if (!(info->status & ~info->mask))
> return 0;
> } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
> pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> info->severity == AER_NONFATAL) {
>
> @@ -1182,6 +1193,8 @@ static int set_device_error_reporting(struct pci_dev *dev, void *data)
> int type = pci_pcie_type(dev);
>
> if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> + (type == PCI_EXP_TYPE_RC_EC) ||
> + (type == PCI_EXP_TYPE_RC_END) ||
Why add RC_END here?
> (type == PCI_EXP_TYPE_UPSTREAM) ||
> (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> if (enable)
> @@ -1206,9 +1219,11 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
> {
> set_device_error_reporting(dev, &enable);
>
> - if (!dev->subordinate)
> - return;
> - pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> + pcie_walk_rcec(dev, set_device_error_reporting, &enable);
> + else if (dev->subordinate)
> + pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
> +
> }
>
> /**
> @@ -1306,6 +1321,11 @@ static int aer_probe(struct pcie_device *dev)
> struct device *device = &dev->device;
> struct pci_dev *port = dev->port;
>
> + /* Limit to Root Ports or Root Complex Event Collectors */
> + if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
> + (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
> + return -ENODEV;
> +
> rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
> if (!rpc)
> return -ENOMEM;
> @@ -1362,7 +1382,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>
> static struct pcie_port_service_driver aerdriver = {
> .name = "aer",
> - .port_type = PCI_EXP_TYPE_ROOT_PORT,
> + .port_type = PCIE_ANY_PORT,
Why this particular change? Seems that is a lot wider than simply
adding RCEC. Obviously we'll then drop out in the aer_probe but it
is still rather inelegant.
> .service = PCIE_PORT_SERVICE_AER,
>
> .probe = aer_probe,
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling
2020-07-27 12:22 ` Jonathan Cameron
@ 2020-07-27 15:19 ` Sean V Kelley
2020-07-27 17:14 ` Jonathan Cameron
0 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 15:19 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, tony.luck, Raj, Ashok, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On 27 Jul 2020, at 5:22, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:21 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
>> and also have the AER capability. So add RCEC support to the current
>> AER
>> service driver and attach the AER service driver to the RCEC device.
>>
>> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>
> A few questions and comments for this patch.
>
> See inline.
>
> Jonathan
>
>
>> ---
>> drivers/pci/pcie/aer.c | 34 +++++++++++++++++++++++++++-------
>> 1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index f1bf06be449e..7cc430c74c46 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -303,7 +303,7 @@ int pci_aer_raw_clear_status(struct pci_dev *dev)
>> return -EIO;
>>
>> port_type = pci_pcie_type(dev);
>> - if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
>> + if (port_type == PCI_EXP_TYPE_ROOT_PORT || port_type ==
>> PCI_EXP_TYPE_RC_EC) {
>> pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status);
>> pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status);
>> }
>> @@ -389,6 +389,12 @@ void pci_aer_init(struct pci_dev *dev)
>> pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR, sizeof(u32) *
>> n);
>>
>> pci_aer_clear_status(dev);
>> +
>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) {
>> + if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_RCEC))
>> + return;
>> + pci_info(dev, "AER: RCEC CAP FOUND and cap_has_rtctl = %d\n", n);
>
> It feels like failing to find an RC_EC extended cap in a RCEC deserved
> a nice strong error message. Perhaps this isn't the right place to do
> it
> though. For that matter, why are we checking for it here?
Sorry, I’ve left an in-development output in the code. Will replace
with a check with more meaningful output elsewhere.
>
>> + }
>> }
>>
>> void pci_aer_exit(struct pci_dev *dev)
>> @@ -577,7 +583,8 @@ static umode_t aer_stats_attrs_are_visible(struct
>> kobject *kobj,
>> if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
>> a == &dev_attr_aer_rootport_total_err_fatal.attr ||
>> a == &dev_attr_aer_rootport_total_err_nonfatal.attr) &&
>
> It is a bit ugly to have these called rootport_total_err etc for the
> rcec.
> Perhaps we should just add additional attributes to reflect we are
> looking at
> an RCEC?
I was trying to avoid any renaming to reduce churn as I did with my
first patch for ACPI / CLX_OSC support.
Will take a look.
>
>> - pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)
>> + ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
>> + (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC)))
>> return 0;
>>
>> return a->mode;
>> @@ -894,7 +901,10 @@ static bool find_source_device(struct pci_dev
>> *parent,
>> if (result)
>> return true;
>>
>> - pci_walk_bus(parent->subordinate, find_device_iter, e_info);
>> + if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
>> + pcie_walk_rcec(parent, find_device_iter, e_info);
>> + else
>> + pci_walk_bus(parent->subordinate, find_device_iter, e_info);
>>
>> if (!e_info->error_dev_num) {
>> pci_info(parent, "can't find device of ID%04x\n", e_info->id);
>> @@ -1030,6 +1040,7 @@ int aer_get_device_error_info(struct pci_dev
>> *dev, struct aer_err_info *info)
>> if (!(info->status & ~info->mask))
>> return 0;
>> } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
>> pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> info->severity == AER_NONFATAL) {
>>
>> @@ -1182,6 +1193,8 @@ static int set_device_error_reporting(struct
>> pci_dev *dev, void *data)
>> int type = pci_pcie_type(dev);
>>
>> if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
>> + (type == PCI_EXP_TYPE_RC_EC) ||
>> + (type == PCI_EXP_TYPE_RC_END) ||
>
> Why add RC_END here?
I’m not clear on your question. Errors can come from RCEC or RCiEPs.
We still need to enable reporting by the RCiEPs.
>
>> (type == PCI_EXP_TYPE_UPSTREAM) ||
>> (type == PCI_EXP_TYPE_DOWNSTREAM)) {
>> if (enable)
>> @@ -1206,9 +1219,11 @@ static void
>> set_downstream_devices_error_reporting(struct pci_dev *dev,
>> {
>> set_device_error_reporting(dev, &enable);
>>
>> - if (!dev->subordinate)
>> - return;
>> - pci_walk_bus(dev->subordinate, set_device_error_reporting,
>> &enable);
>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
>> + pcie_walk_rcec(dev, set_device_error_reporting, &enable);
>> + else if (dev->subordinate)
>> + pci_walk_bus(dev->subordinate, set_device_error_reporting,
>> &enable);
>> +
>> }
>>
>> /**
>> @@ -1306,6 +1321,11 @@ static int aer_probe(struct pcie_device *dev)
>> struct device *device = &dev->device;
>> struct pci_dev *port = dev->port;
>>
>> + /* Limit to Root Ports or Root Complex Event Collectors */
>> + if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
>> + (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
>> + return -ENODEV;
>> +
>> rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
>> if (!rpc)
>> return -ENOMEM;
>> @@ -1362,7 +1382,7 @@ static pci_ers_result_t aer_root_reset(struct
>> pci_dev *dev)
>>
>> static struct pcie_port_service_driver aerdriver = {
>> .name = "aer",
>> - .port_type = PCI_EXP_TYPE_ROOT_PORT,
>> + .port_type = PCIE_ANY_PORT,
>
> Why this particular change? Seems that is a lot wider than simply
> adding RCEC. Obviously we'll then drop out in the aer_probe but it
> is still rather inelegant.
In order to extend the service drivers to non-root-port devices (i.e.,
RCEC), the simple path appeared to only require setting the type to
ANY_PORT and catching the needed types arriving in the probe. Would you
prefer extending to a type2? I’m not sure how one is more elegant
than another but open to that approach. However, this seems to require
less code perhaps and seems consistent with most ‘drop-out’
conditional patterns in the kernel. The same applies to pme.
Thanks,
Sean
>
>> .service = PCIE_PORT_SERVICE_AER,
>>
>> .probe = aer_probe,
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling
2020-07-27 15:19 ` Sean V Kelley
@ 2020-07-27 17:14 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 17:14 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, tony.luck, Raj, Ashok, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel, Qiuxu Zhuo
On Mon, 27 Jul 2020 08:19:39 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> On 27 Jul 2020, at 5:22, Jonathan Cameron wrote:
>
> > On Fri, 24 Jul 2020 10:22:21 -0700
> > Sean V Kelley <sean.v.kelley@intel.com> wrote:
> >
> >> The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
> >> and also have the AER capability. So add RCEC support to the current
> >> AER
> >> service driver and attach the AER service driver to the RCEC device.
> >>
> >> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> >> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >
> > A few questions and comments for this patch.
> >
> > See inline.
> >
> > Jonathan
> >
> >
> >> ---
> >> drivers/pci/pcie/aer.c | 34 +++++++++++++++++++++++++++-------
> >> 1 file changed, 27 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >> index f1bf06be449e..7cc430c74c46 100644
> >> --- a/drivers/pci/pcie/aer.c
> >> +++ b/drivers/pci/pcie/aer.c
> >> @@ -303,7 +303,7 @@ int pci_aer_raw_clear_status(struct pci_dev *dev)
> >> return -EIO;
> >>
> >> port_type = pci_pcie_type(dev);
> >> - if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
> >> + if (port_type == PCI_EXP_TYPE_ROOT_PORT || port_type ==
> >> PCI_EXP_TYPE_RC_EC) {
> >> pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status);
> >> pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status);
> >> }
> >> @@ -389,6 +389,12 @@ void pci_aer_init(struct pci_dev *dev)
> >> pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ERR, sizeof(u32) *
> >> n);
> >>
> >> pci_aer_clear_status(dev);
> >> +
> >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) {
> >> + if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_RCEC))
> >> + return;
> >> + pci_info(dev, "AER: RCEC CAP FOUND and cap_has_rtctl = %d\n", n);
> >
> > It feels like failing to find an RC_EC extended cap in a RCEC deserved
> > a nice strong error message. Perhaps this isn't the right place to do
> > it
> > though. For that matter, why are we checking for it here?
>
> Sorry, I’ve left an in-development output in the code. Will replace
> with a check with more meaningful output elsewhere.
>
> >
> >> + }
> >> }
> >>
> >> void pci_aer_exit(struct pci_dev *dev)
> >> @@ -577,7 +583,8 @@ static umode_t aer_stats_attrs_are_visible(struct
> >> kobject *kobj,
> >> if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
> >> a == &dev_attr_aer_rootport_total_err_fatal.attr ||
> >> a == &dev_attr_aer_rootport_total_err_nonfatal.attr) &&
> >
> > It is a bit ugly to have these called rootport_total_err etc for the
> > rcec.
> > Perhaps we should just add additional attributes to reflect we are
> > looking at
> > an RCEC?
>
> I was trying to avoid any renaming to reduce churn as I did with my
> first patch for ACPI / CLX_OSC support.
> Will take a look.
>
> >
> >> - pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)
> >> + ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
> >> + (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC)))
> >> return 0;
> >>
> >> return a->mode;
> >> @@ -894,7 +901,10 @@ static bool find_source_device(struct pci_dev
> >> *parent,
> >> if (result)
> >> return true;
> >>
> >> - pci_walk_bus(parent->subordinate, find_device_iter, e_info);
> >> + if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
> >> + pcie_walk_rcec(parent, find_device_iter, e_info);
> >> + else
> >> + pci_walk_bus(parent->subordinate, find_device_iter, e_info);
> >>
> >> if (!e_info->error_dev_num) {
> >> pci_info(parent, "can't find device of ID%04x\n", e_info->id);
> >> @@ -1030,6 +1040,7 @@ int aer_get_device_error_info(struct pci_dev
> >> *dev, struct aer_err_info *info)
> >> if (!(info->status & ~info->mask))
> >> return 0;
> >> } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> >> + pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
> >> pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> >> info->severity == AER_NONFATAL) {
> >>
> >> @@ -1182,6 +1193,8 @@ static int set_device_error_reporting(struct
> >> pci_dev *dev, void *data)
> >> int type = pci_pcie_type(dev);
> >>
> >> if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> >> + (type == PCI_EXP_TYPE_RC_EC) ||
> >> + (type == PCI_EXP_TYPE_RC_END) ||
> >
> > Why add RC_END here?
>
> I’m not clear on your question. Errors can come from RCEC or RCiEPs.
> We still need to enable reporting by the RCiEPs.
I was curious to see that we need it in this code path for an RCiEP but
not for a normal EP. From a quick glance it looks like that is often
done in the drivers for the EPs themselves rather than here.
>
> >
> >> (type == PCI_EXP_TYPE_UPSTREAM) ||
> >> (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> >> if (enable)
> >> @@ -1206,9 +1219,11 @@ static void
> >> set_downstream_devices_error_reporting(struct pci_dev *dev,
> >> {
> >> set_device_error_reporting(dev, &enable);
> >>
> >> - if (!dev->subordinate)
> >> - return;
> >> - pci_walk_bus(dev->subordinate, set_device_error_reporting,
> >> &enable);
> >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> >> + pcie_walk_rcec(dev, set_device_error_reporting, &enable);
> >> + else if (dev->subordinate)
> >> + pci_walk_bus(dev->subordinate, set_device_error_reporting,
> >> &enable);
> >> +
> >> }
> >>
> >> /**
> >> @@ -1306,6 +1321,11 @@ static int aer_probe(struct pcie_device *dev)
> >> struct device *device = &dev->device;
> >> struct pci_dev *port = dev->port;
> >>
> >> + /* Limit to Root Ports or Root Complex Event Collectors */
> >> + if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
> >> + (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
> >> + return -ENODEV;
> >> +
> >> rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
> >> if (!rpc)
> >> return -ENOMEM;
> >> @@ -1362,7 +1382,7 @@ static pci_ers_result_t aer_root_reset(struct
> >> pci_dev *dev)
> >>
> >> static struct pcie_port_service_driver aerdriver = {
> >> .name = "aer",
> >> - .port_type = PCI_EXP_TYPE_ROOT_PORT,
> >> + .port_type = PCIE_ANY_PORT,
> >
> > Why this particular change? Seems that is a lot wider than simply
> > adding RCEC. Obviously we'll then drop out in the aer_probe but it
> > is still rather inelegant.
>
> In order to extend the service drivers to non-root-port devices (i.e.,
> RCEC), the simple path appeared to only require setting the type to
> ANY_PORT and catching the needed types arriving in the probe. Would you
> prefer extending to a type2? I’m not sure how one is more elegant
> than another but open to that approach. However, this seems to require
> less code perhaps and seems consistent with most ‘drop-out’
> conditional patterns in the kernel. The same applies to pme.
I'd miss understood this bit. It's fine as you have it here.
Jonathan
>
> Thanks,
>
> Sean
>
>
> >
> >> .service = PCIE_PORT_SERVICE_AER,
> >>
> >> .probe = aer_probe,
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 8/9] PCI/PME: Add RCEC PME handling
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (6 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 7/9] PCI/AER: Add RCEC AER handling Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-08-04 8:35 ` Jay Fang
2020-07-24 17:22 ` [RFC PATCH 9/9] PCI/AER: Add RCEC AER error injection support Sean V Kelley
2020-07-27 12:37 ` [RFC PATCH 0/9] Add RCEC handling to PCI/AER Jonathan Cameron
9 siblings, 1 reply; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Sean V Kelley, Qiuxu Zhuo
The Root Complex Event Collectors(RCEC) appear as peers of Root Ports
and also have the PME capability. So add RCEC support to the current PME
service driver and attach the PME service driver to the RCEC device.
Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
drivers/pci/pcie/pme.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 6a32970bb731..87799166c96a 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -310,7 +310,10 @@ static int pcie_pme_can_wakeup(struct pci_dev *dev, void *ign)
static void pcie_pme_mark_devices(struct pci_dev *port)
{
pcie_pme_can_wakeup(port, NULL);
- if (port->subordinate)
+
+ if (pci_pcie_type(port) == PCI_EXP_TYPE_RC_EC)
+ pcie_walk_rcec(port, pcie_pme_can_wakeup, NULL);
+ else if (port->subordinate)
pci_walk_bus(port->subordinate, pcie_pme_can_wakeup, NULL);
}
@@ -320,10 +323,15 @@ static void pcie_pme_mark_devices(struct pci_dev *port)
*/
static int pcie_pme_probe(struct pcie_device *srv)
{
- struct pci_dev *port;
+ struct pci_dev *port = srv->port;
struct pcie_pme_service_data *data;
int ret;
+ /* Limit to Root Ports or Root Complex Event Collectors */
+ if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
+ (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
+ return -ENODEV;
+
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
@@ -333,7 +341,6 @@ static int pcie_pme_probe(struct pcie_device *srv)
data->srv = srv;
set_service_data(srv, data);
- port = srv->port;
pcie_pme_interrupt_enable(port, false);
pcie_clear_root_pme_status(port);
@@ -445,7 +452,7 @@ static void pcie_pme_remove(struct pcie_device *srv)
static struct pcie_port_service_driver pcie_pme_driver = {
.name = "pcie_pme",
- .port_type = PCI_EXP_TYPE_ROOT_PORT,
+ .port_type = PCIE_ANY_PORT,
.service = PCIE_PORT_SERVICE_PME,
.probe = pcie_pme_probe,
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 8/9] PCI/PME: Add RCEC PME handling
2020-07-24 17:22 ` [RFC PATCH 8/9] PCI/PME: Add RCEC PME handling Sean V Kelley
@ 2020-08-04 8:35 ` Jay Fang
2020-08-04 9:47 ` Jonathan Cameron
0 siblings, 1 reply; 37+ messages in thread
From: Jay Fang @ 2020-08-04 8:35 UTC (permalink / raw)
To: Sean V Kelley, bhelgaas, Jonathan.Cameron, rjw, ashok.raj,
tony.luck, sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo
在 2020/7/25 1:22, Sean V Kelley 写道:
> The Root Complex Event Collectors(RCEC) appear as peers of Root Ports
> and also have the PME capability. So add RCEC support to the current PME
> service driver and attach the PME service driver to the RCEC device.
>
> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> ---
> drivers/pci/pcie/pme.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index 6a32970bb731..87799166c96a 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -310,7 +310,10 @@ static int pcie_pme_can_wakeup(struct pci_dev *dev, void *ign)
> static void pcie_pme_mark_devices(struct pci_dev *port)
> {
> pcie_pme_can_wakeup(port, NULL);
> - if (port->subordinate)
> +
> + if (pci_pcie_type(port) == PCI_EXP_TYPE_RC_EC)
> + pcie_walk_rcec(port, pcie_pme_can_wakeup, NULL);
> + else if (port->subordinate)
> pci_walk_bus(port->subordinate, pcie_pme_can_wakeup, NULL);
> }
>
> @@ -320,10 +323,15 @@ static void pcie_pme_mark_devices(struct pci_dev *port)
> */
> static int pcie_pme_probe(struct pcie_device *srv)
> {
> - struct pci_dev *port;
> + struct pci_dev *port = srv->port;
> struct pcie_pme_service_data *data;
> int ret;
>
> + /* Limit to Root Ports or Root Complex Event Collectors */
> + if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
> + (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
> + return -ENODEV;
> +
> data = kzalloc(sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
> @@ -333,7 +341,6 @@ static int pcie_pme_probe(struct pcie_device *srv)
> data->srv = srv;
> set_service_data(srv, data);
>
> - port = srv->port;
> pcie_pme_interrupt_enable(port, false);
> pcie_clear_root_pme_status(port);
>
> @@ -445,7 +452,7 @@ static void pcie_pme_remove(struct pcie_device *srv)
>
> static struct pcie_port_service_driver pcie_pme_driver = {
> .name = "pcie_pme",
> - .port_type = PCI_EXP_TYPE_ROOT_PORT,
> + .port_type = PCIE_ANY_PORT,
Maybe we can use port_type for driver matching. There is no need
to check the type of port in pcie_pme_probe function.
Jay
> .service = PCIE_PORT_SERVICE_PME,
>
> .probe = pcie_pme_probe,
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 8/9] PCI/PME: Add RCEC PME handling
2020-08-04 8:35 ` Jay Fang
@ 2020-08-04 9:47 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2020-08-04 9:47 UTC (permalink / raw)
To: Jay Fang
Cc: Sean V Kelley, bhelgaas, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy, linux-pci, linux-kernel, Qiuxu Zhuo
On Tue, 4 Aug 2020 16:35:59 +0800
Jay Fang <f.fangjian@huawei.com> wrote:
> 在 2020/7/25 1:22, Sean V Kelley 写道:
> > The Root Complex Event Collectors(RCEC) appear as peers of Root Ports
> > and also have the PME capability. So add RCEC support to the current PME
> > service driver and attach the PME service driver to the RCEC device.
> >
> > Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > ---
> > drivers/pci/pcie/pme.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> > index 6a32970bb731..87799166c96a 100644
> > --- a/drivers/pci/pcie/pme.c
> > +++ b/drivers/pci/pcie/pme.c
> > @@ -310,7 +310,10 @@ static int pcie_pme_can_wakeup(struct pci_dev *dev, void *ign)
> > static void pcie_pme_mark_devices(struct pci_dev *port)
> > {
> > pcie_pme_can_wakeup(port, NULL);
> > - if (port->subordinate)
> > +
> > + if (pci_pcie_type(port) == PCI_EXP_TYPE_RC_EC)
> > + pcie_walk_rcec(port, pcie_pme_can_wakeup, NULL);
> > + else if (port->subordinate)
> > pci_walk_bus(port->subordinate, pcie_pme_can_wakeup, NULL);
> > }
> >
> > @@ -320,10 +323,15 @@ static void pcie_pme_mark_devices(struct pci_dev *port)
> > */
> > static int pcie_pme_probe(struct pcie_device *srv)
> > {
> > - struct pci_dev *port;
> > + struct pci_dev *port = srv->port;
> > struct pcie_pme_service_data *data;
> > int ret;
> >
> > + /* Limit to Root Ports or Root Complex Event Collectors */
> > + if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
> > + (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
> > + return -ENODEV;
> > +
> > data = kzalloc(sizeof(*data), GFP_KERNEL);
> > if (!data)
> > return -ENOMEM;
> > @@ -333,7 +341,6 @@ static int pcie_pme_probe(struct pcie_device *srv)
> > data->srv = srv;
> > set_service_data(srv, data);
> >
> > - port = srv->port;
> > pcie_pme_interrupt_enable(port, false);
> > pcie_clear_root_pme_status(port);
> >
> > @@ -445,7 +452,7 @@ static void pcie_pme_remove(struct pcie_device *srv)
> >
> > static struct pcie_port_service_driver pcie_pme_driver = {
> > .name = "pcie_pme",
> > - .port_type = PCI_EXP_TYPE_ROOT_PORT,
> > + .port_type = PCIE_ANY_PORT,
> Maybe we can use port_type for driver matching. There is no need
> to check the type of port in pcie_pme_probe function.
>
I walked into the same hole for the AER case.
port_type is effectively an enum so there is no way of specifying several
types unless you want to register different instances of pcie_port_service_driver
and that isn't currently possible.
The PCIE_ANY_PORT is a special case value.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/pci_regs.h#L477
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L1651
So odd corner case, but I think this is the right solution. Anything better
would require a lot more code to change.
Jonathan
>
> Jay
>
> > .service = PCIE_PORT_SERVICE_PME,
> >
> > .probe = pcie_pme_probe,
> >
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH 9/9] PCI/AER: Add RCEC AER error injection support
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (7 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 8/9] PCI/PME: Add RCEC PME handling Sean V Kelley
@ 2020-07-24 17:22 ` Sean V Kelley
2020-07-27 12:37 ` [RFC PATCH 0/9] Add RCEC handling to PCI/AER Jonathan Cameron
9 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-24 17:22 UTC (permalink / raw)
To: bhelgaas, Jonathan.Cameron, rjw, ashok.raj, tony.luck,
sathyanarayanan.kuppuswamy
Cc: linux-pci, linux-kernel, Qiuxu Zhuo, Sean V Kelley
From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
and also have the AER capability. So add RCEC support to the current AER
error injection driver.
Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
drivers/pci/pcie/aer_inject.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
index 21cc3d3387f7..5264847bd714 100644
--- a/drivers/pci/pcie/aer_inject.c
+++ b/drivers/pci/pcie/aer_inject.c
@@ -333,8 +333,11 @@ static int aer_inject(struct aer_error_inj *einj)
if (!dev)
return -ENODEV;
rpdev = pcie_find_root_port(dev);
+ /* If Root port not found, try to find an RCEC */
+ if (!rpdev)
+ rpdev = dev->rcec;
if (!rpdev) {
- pci_err(dev, "Root port not found\n");
+ pci_err(dev, "Root port or RCEC not found\n");
ret = -ENODEV;
goto out_put;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH 0/9] Add RCEC handling to PCI/AER
2020-07-24 17:22 [RFC PATCH 0/9] Add RCEC handling to PCI/AER Sean V Kelley
` (8 preceding siblings ...)
2020-07-24 17:22 ` [RFC PATCH 9/9] PCI/AER: Add RCEC AER error injection support Sean V Kelley
@ 2020-07-27 12:37 ` Jonathan Cameron
2020-07-27 14:56 ` Sean V Kelley
9 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2020-07-27 12:37 UTC (permalink / raw)
To: Sean V Kelley
Cc: bhelgaas, rjw, ashok.raj, tony.luck, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
On Fri, 24 Jul 2020 10:22:14 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:
> Root Complex Event Collectors (RCEC) provide support for terminating error
> and PME messages from Root Complex Integrated Endpoints (RCiEPs). An RCEC
> resides on a Bus in the Root Complex. Multiple RCECs can in fact reside on
> a single bus. An RCEC will explicitly declare supported RCiEPs through the
> Root Complex Endpoint Association Extended Capability.
>
> (See PCIe 5.0-1, sections 1.3.2.3 (RCiEP), and 7.9.10 (RCEC Ext. Cap.))
>
> The kernel lacks handling for these RCECs and the error messages received
> from their respective associated RCiEPs. More recently, a new CPU
> interconnect, Compute eXpress Link (CXL) depends on RCEC capabilities for
> purposes of error messaging from CXL 1.1 supported RCiEP devices.
>
> DocLink: https://www.computeexpresslink.org/
>
> This use case is not limited to CXL. Existing hardware today includes
> support for RCECs, such as the Denverton microserver product
> family. Future hardware will be forthcoming.
>
> (See Intel Document, Order number: 33061-003US)
>
> So services such as AER or PME could be associated with an RCEC driver.
> In the case of CXL, if an RCiEP (i.e., CXL 1.1 device) is associated with a
> platform's RCEC it shall signal PME and AER error conditions through that
> RCEC.
>
> Towards the above use cases, add the missing RCEC class and extend the
> PCIe Root Port and service drivers to allow association of RCiEPs to their
> respective parent RCEC and facilitate handling of terminating error and PME
> messages.
Silly question number 1. Why an RFC? I always find it helps to highlight which
bits you are unsure on / want particular input on.
Otherwise, I've left the PME and error injection patches as I don't really know
anything about those two paths.
I'll fire up my APEI etc test VMs in the nex day or so and report back if there
any problems with that case (fairly sure there is one in patch 6, highlighted in
review but it is possible I've missed others.
It all seems to have come together rather simpler than I was expecting which is
great!
Thanks,
Jonathan
>
> TESTING:
>
> Results:
> 1) Show RCiEPs which are associated with RCECs:
> Run dmesg | grep "RCiEP"
> Log:
> [ 8.981698] pcieport 0000:e8:00.4: RCiEP(under an RCEC) 0000:e8:01.0
> [ 8.988830] pcieport 0000:e8:00.4: RCiEP(under an RCEC) 0000:e8:02.0
> [ 8.995956] pcieport 0000:e8:00.4: RCiEP(under an RCEC) 0000:e9:00.0
> [ 9.023034] pcieport 0000:ed:00.4: RCiEP(under an RCEC) 0000:ed:01.0
> [ 9.030159] pcieport 0000:ed:00.4: RCiEP(under an RCEC) 0000:ed:02.0
> [ 9.037282] pcieport 0000:ed:00.4: RCiEP(under an RCEC) 0000:ee:00.0
> [ 9.064294] pcieport 0000:f2:00.4: RCiEP(under an RCEC) 0000:f2:01.0
> [ 9.071409] pcieport 0000:f2:00.4: RCiEP(under an RCEC) 0000:f2:02.0
> [ 9.078526] pcieport 0000:f2:00.4: RCiEP(under an RCEC) 0000:f3:00.0
> [ 9.105535] pcieport 0000:f7:00.4: RCiEP(under an RCEC) 0000:f7:01.0
> [ 9.112652] pcieport 0000:f7:00.4: RCiEP(under an RCEC) 0000:f7:02.0
> [ 9.119774] pcieport 0000:f7:00.4: RCiEP(under an RCEC) 0000:f8:00.0
>
> 2) Inject a correctable error to the RCiEP 0000:e9:00.0
> Run ./aer_inject <a parameter file as below>:
> AER
> PCI_ID 0000:e9:00.0
> COR_STATUS BAD_TLP
> HEADER_LOG 0 1 2 3
>
> Log:
> [ 253.248362] pcieport 0000:e8:00.4: aer_inject: Injecting errors 00000040/00000000 into device 0000:e9:00.0
> [ 253.260656] pcieport 0000:e8:00.4: AER: Corrected error received: 0000:e9:00.0
> [ 253.269919] pci 0000:e9:00.0: AER: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver ID)
> [ 253.282549] pci 0000:e9:00.0: AER: device [8086:4940] error status/mask=00000040/00002000
> [ 253.293937] pci 0000:e9:00.0: AER: [ 6] BadTLP
>
> 3) Inject a non-fatal error to the RCiEP 0000:e8:01.0
> Run ./aer_inject <a parameter file as below>:
> AER
> PCI_ID 0000:e8:01.0
> UNCOR_STATUS COMP_ABORT
> HEADER_LOG 0 1 2 3
>
> Log:
> [ 288.405326] pcieport 0000:e8:00.4: aer_inject: Injecting errors 00000000/00008000 into device 0000:e8:01.0
> [ 288.416881] pcieport 0000:e8:00.4: AER: Uncorrected (Non-Fatal) error received: 0000:e8:01.0
> [ 288.427487] igen6_edac 0000:e8:01.0: AER: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Completer ID)
> [ 288.442098] igen6_edac 0000:e8:01.0: AER: device [8086:0b25] error status/mask=00008000/00100000
> [ 288.452869] igen6_edac 0000:e8:01.0: AER: [15] CmpltAbrt
> [ 288.461118] igen6_edac 0000:e8:01.0: AER: TLP Header: 00000000 00000001 00000002 00000003
> [ 288.471192] igen6_edac 0000:e8:01.0: AER: device recovery successful
>
> 4) Inject a fatal error to the RCiEP 0000:ed:01.0
> Run ./aer_inject <a parameter file as below>:
> AER
> PCI_ID 0000:ed:01.0
> UNCOR_STATUS MALF_TLP
> HEADER_LOG 0 1 2 3
>
> Log:
> [ 535.537281] pcieport 0000:ed:00.4: aer_inject: Injecting errors 00000000/00040000 into device 0000:ed:01.0
> [ 535.551911] pcieport 0000:ed:00.4: AER: Uncorrected (Fatal) error received: 0000:ed:01.0
> [ 535.561556] igen6_edac 0000:ed:01.0: AER: PCIe Bus Error: severity=Uncorrected (Fatal), type=Inaccessible, (Unregistered Agent ID)
> [ 535.684964] igen6_edac 0000:ed:01.0: AER: device recovery successful
>
>
> Jonathan Cameron (1):
> PCI/AER: Extend AER error handling to RCECs
>
> Qiuxu Zhuo (6):
> pci_ids: Add class code and extended capability for RCEC
> PCI: Extend Root Port Driver to support RCEC
> PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC
> PCI/AER: Apply function level reset to RCiEP on fatal error
> PCI: Add 'rcec' field to pci_dev for associated RCiEPs
> PCI/AER: Add RCEC AER error injection support
>
> Sean V Kelley (2):
> PCI/AER: Add RCEC AER handling
> PCI/PME: Add RCEC PME handling
>
> drivers/pci/pcie/aer.c | 43 ++++++++++++-----
> drivers/pci/pcie/aer_inject.c | 5 +-
> drivers/pci/pcie/err.c | 85 +++++++++++++++++++++++++++------
> drivers/pci/pcie/pme.c | 15 ++++--
> drivers/pci/pcie/portdrv.h | 2 +
> drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++
> drivers/pci/pcie/portdrv_pci.c | 20 +++++++-
> include/linux/pci.h | 3 ++
> include/linux/pci_ids.h | 1 +
> include/uapi/linux/pci_regs.h | 7 +++
> 10 files changed, 232 insertions(+), 31 deletions(-)
>
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH 0/9] Add RCEC handling to PCI/AER
2020-07-27 12:37 ` [RFC PATCH 0/9] Add RCEC handling to PCI/AER Jonathan Cameron
@ 2020-07-27 14:56 ` Sean V Kelley
0 siblings, 0 replies; 37+ messages in thread
From: Sean V Kelley @ 2020-07-27 14:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: bhelgaas, rjw, tony.luck, Raj, Ashok, sathyanarayanan.kuppuswamy,
linux-pci, linux-kernel
Hi Jonathan,
On 27 Jul 2020, at 5:37, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 10:22:14 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> Root Complex Event Collectors (RCEC) provide support for terminating
>> error
>> and PME messages from Root Complex Integrated Endpoints (RCiEPs). An
>> RCEC
>> resides on a Bus in the Root Complex. Multiple RCECs can in fact
>> reside on
>> a single bus. An RCEC will explicitly declare supported RCiEPs
>> through the
>> Root Complex Endpoint Association Extended Capability.
>>
>> (See PCIe 5.0-1, sections 1.3.2.3 (RCiEP), and 7.9.10 (RCEC Ext.
>> Cap.))
>>
>> The kernel lacks handling for these RCECs and the error messages
>> received
>> from their respective associated RCiEPs. More recently, a new CPU
>> interconnect, Compute eXpress Link (CXL) depends on RCEC capabilities
>> for
>> purposes of error messaging from CXL 1.1 supported RCiEP devices.
>>
>> DocLink: https://www.computeexpresslink.org/
>>
>> This use case is not limited to CXL. Existing hardware today includes
>> support for RCECs, such as the Denverton microserver product
>> family. Future hardware will be forthcoming.
>>
>> (See Intel Document, Order number: 33061-003US)
>>
>> So services such as AER or PME could be associated with an RCEC
>> driver.
>> In the case of CXL, if an RCiEP (i.e., CXL 1.1 device) is associated
>> with a
>> platform's RCEC it shall signal PME and AER error conditions through
>> that
>> RCEC.
>>
>> Towards the above use cases, add the missing RCEC class and extend
>> the
>> PCIe Root Port and service drivers to allow association of RCiEPs to
>> their
>> respective parent RCEC and facilitate handling of terminating error
>> and PME
>> messages.
>
> Silly question number 1. Why an RFC? I always find it helps to
> highlight which
> bits you are unsure on / want particular input on.
I suppose it was because we were continuing the conversation from
discussion on the mailing list.
And I was not fully sure about the impact to your use case. No worries,
I will remove it.
>
> Otherwise, I've left the PME and error injection patches as I don't
> really know
> anything about those two paths.
>
> I'll fire up my APEI etc test VMs in the nex day or so and report back
> if there
> any problems with that case (fairly sure there is one in patch 6,
> highlighted in
> review but it is possible I've missed others.
>
> It all seems to have come together rather simpler than I was expecting
> which is
> great!
Sounds good, looking forward to your feedback.
Sean
>
> Thanks,
>
> Jonathan
>
>
>>
>> TESTING:
>>
>> Results:
>> 1) Show RCiEPs which are associated with RCECs:
>> Run dmesg | grep "RCiEP"
>> Log:
>> [ 8.981698] pcieport 0000:e8:00.4: RCiEP(under an RCEC)
>> 0000:e8:01.0
>> [ 8.988830] pcieport 0000:e8:00.4: RCiEP(under an RCEC)
>> 0000:e8:02.0
>> [ 8.995956] pcieport 0000:e8:00.4: RCiEP(under an RCEC)
>> 0000:e9:00.0
>> [ 9.023034] pcieport 0000:ed:00.4: RCiEP(under an RCEC)
>> 0000:ed:01.0
>> [ 9.030159] pcieport 0000:ed:00.4: RCiEP(under an RCEC)
>> 0000:ed:02.0
>> [ 9.037282] pcieport 0000:ed:00.4: RCiEP(under an RCEC)
>> 0000:ee:00.0
>> [ 9.064294] pcieport 0000:f2:00.4: RCiEP(under an RCEC)
>> 0000:f2:01.0
>> [ 9.071409] pcieport 0000:f2:00.4: RCiEP(under an RCEC)
>> 0000:f2:02.0
>> [ 9.078526] pcieport 0000:f2:00.4: RCiEP(under an RCEC)
>> 0000:f3:00.0
>> [ 9.105535] pcieport 0000:f7:00.4: RCiEP(under an RCEC)
>> 0000:f7:01.0
>> [ 9.112652] pcieport 0000:f7:00.4: RCiEP(under an RCEC)
>> 0000:f7:02.0
>> [ 9.119774] pcieport 0000:f7:00.4: RCiEP(under an RCEC)
>> 0000:f8:00.0
>>
>> 2) Inject a correctable error to the RCiEP 0000:e9:00.0
>> Run ./aer_inject <a parameter file as below>:
>> AER
>> PCI_ID 0000:e9:00.0
>> COR_STATUS BAD_TLP
>> HEADER_LOG 0 1 2 3
>>
>> Log:
>> [ 253.248362] pcieport 0000:e8:00.4: aer_inject: Injecting errors
>> 00000040/00000000 into device 0000:e9:00.0
>> [ 253.260656] pcieport 0000:e8:00.4: AER: Corrected error received:
>> 0000:e9:00.0
>> [ 253.269919] pci 0000:e9:00.0: AER: PCIe Bus Error:
>> severity=Corrected, type=Data Link Layer, (Receiver ID)
>> [ 253.282549] pci 0000:e9:00.0: AER: device [8086:4940] error
>> status/mask=00000040/00002000
>> [ 253.293937] pci 0000:e9:00.0: AER: [ 6] BadTLP
>>
>> 3) Inject a non-fatal error to the RCiEP 0000:e8:01.0
>> Run ./aer_inject <a parameter file as below>:
>> AER
>> PCI_ID 0000:e8:01.0
>> UNCOR_STATUS COMP_ABORT
>> HEADER_LOG 0 1 2 3
>>
>> Log:
>> [ 288.405326] pcieport 0000:e8:00.4: aer_inject: Injecting errors
>> 00000000/00008000 into device 0000:e8:01.0
>> [ 288.416881] pcieport 0000:e8:00.4: AER: Uncorrected (Non-Fatal)
>> error received: 0000:e8:01.0
>> [ 288.427487] igen6_edac 0000:e8:01.0: AER: PCIe Bus Error:
>> severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Completer
>> ID)
>> [ 288.442098] igen6_edac 0000:e8:01.0: AER: device [8086:0b25]
>> error status/mask=00008000/00100000
>> [ 288.452869] igen6_edac 0000:e8:01.0: AER: [15] CmpltAbrt
>> [ 288.461118] igen6_edac 0000:e8:01.0: AER: TLP Header: 00000000
>> 00000001 00000002 00000003
>> [ 288.471192] igen6_edac 0000:e8:01.0: AER: device recovery
>> successful
>>
>> 4) Inject a fatal error to the RCiEP 0000:ed:01.0
>> Run ./aer_inject <a parameter file as below>:
>> AER
>> PCI_ID 0000:ed:01.0
>> UNCOR_STATUS MALF_TLP
>> HEADER_LOG 0 1 2 3
>>
>> Log:
>> [ 535.537281] pcieport 0000:ed:00.4: aer_inject: Injecting errors
>> 00000000/00040000 into device 0000:ed:01.0
>> [ 535.551911] pcieport 0000:ed:00.4: AER: Uncorrected (Fatal) error
>> received: 0000:ed:01.0
>> [ 535.561556] igen6_edac 0000:ed:01.0: AER: PCIe Bus Error:
>> severity=Uncorrected (Fatal), type=Inaccessible, (Unregistered Agent
>> ID)
>> [ 535.684964] igen6_edac 0000:ed:01.0: AER: device recovery
>> successful
>>
>>
>> Jonathan Cameron (1):
>> PCI/AER: Extend AER error handling to RCECs
>>
>> Qiuxu Zhuo (6):
>> pci_ids: Add class code and extended capability for RCEC
>> PCI: Extend Root Port Driver to support RCEC
>> PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with
>> RCEC
>> PCI/AER: Apply function level reset to RCiEP on fatal error
>> PCI: Add 'rcec' field to pci_dev for associated RCiEPs
>> PCI/AER: Add RCEC AER error injection support
>>
>> Sean V Kelley (2):
>> PCI/AER: Add RCEC AER handling
>> PCI/PME: Add RCEC PME handling
>>
>> drivers/pci/pcie/aer.c | 43 ++++++++++++-----
>> drivers/pci/pcie/aer_inject.c | 5 +-
>> drivers/pci/pcie/err.c | 85
>> +++++++++++++++++++++++++++------
>> drivers/pci/pcie/pme.c | 15 ++++--
>> drivers/pci/pcie/portdrv.h | 2 +
>> drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++
>> drivers/pci/pcie/portdrv_pci.c | 20 +++++++-
>> include/linux/pci.h | 3 ++
>> include/linux/pci_ids.h | 1 +
>> include/uapi/linux/pci_regs.h | 7 +++
>> 10 files changed, 232 insertions(+), 31 deletions(-)
>>
>> --
>> 2.27.0
>>
^ permalink raw reply [flat|nested] 37+ messages in thread