[RFC PATCH 0/6] Add support for root port RAS error handling

[RFC PATCH 0/6] Add support for root port RAS error handling

[Li Ming]

Protocol errors signaled to a CXL root port may be captured by a Root
Complex Event Collector(RCEC). If those errors are not cleared and
reported the system owner loses forensic information for system failure
analysis.

Per CXL r3.1 section 9.18.1.5, the recommendation for this case from CXL
specification is the 'Else' statement in 'IMPLEMENTATION NODE' under
'Table 9-24 RDPAS Structure':

	"Probe all CXL Downstream Ports and determine whether they have logged an
	error in the CXL.io or CXL.cachemem status registers."

The CXL subsystem already supports RCH RAS Error handling that has a
dependency on the RCEC. Reuse and extend that RCH topoogy support to
handle reported errors in the VH topology case. The implementation is
composed of:
* Provide a new interface from RCEC side to support walk all devices
  under RCEC and RCEC associated bus range. PCIe AER core uses this
  interface to walk all CXL endpoints and all CXL root ports under the
  bus ranges.

* Update the PCIe AER core to enable Uncorrectable Internal Errors and
  Correctable Internal Errors report for root ports.

* Invoke the cxl_pci error handler for RCEC reported errors.

* Handle root-port errors in the cxl_pci handler when the device is
  direct attached.

The implementation is only for above case without CXL switch, still
remain two opens to be discussed.
1. Is it compatible for CXL switch port error handling?
CXL switch port error handling proposal has not yet been finalized.
Should confirm that this implementation will be compatible with that.

2. How to handle the case which CXL root port reported CXL.CM protocol
erros by itself?
Not support for this case in the patchset at present, my opinion is that
invoking the cxl_pci handle to deal with such case as well.

base-commit: 73bf93edeeea866b0b6efbc8d2595bdaaba7f1a5 branch: next

Li Ming (6):
  PCI/RCEC: Introduce pcie_walk_rcec_all()
  PCI/CXL: A new attribute to indicate if a host bridge is CXL capable
  PCI/AER: Enable RCEC to report internal error for CXL root port
  PCI/AER: Support to handle errors detected by CXL root port
  cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()
  cxl/pci: Add support for the RAS handling of RCEC captured errors on
    RP

 drivers/acpi/pci_root.c |  1 +
 drivers/cxl/core/pci.c  | 89 +++++++++++++++++++++++++++--------------
 drivers/cxl/core/port.c |  9 +++++
 drivers/cxl/cxl.h       |  2 +
 drivers/cxl/mem.c       |  5 +--
 drivers/cxl/pci.c       | 12 +++---
 drivers/pci/pci.h       |  6 +++
 drivers/pci/pcie/aer.c  | 44 +++++++++++++-------
 drivers/pci/pcie/rcec.c | 44 +++++++++++++++++++-
 include/linux/pci.h     |  1 +
 10 files changed, 155 insertions(+), 58 deletions(-)

-- 
2.40.1

[RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Li Ming]

PCIe RCEC core only provides pcie_walk_rcec() to walk all RCiEP devices
associating with RCEC, but CXL subsystem needs a helper function which
can walk all devices in RCEC associated bus range other than RCiEPs for
below RAS error case.

CXL r3.1 section 12.2.2 mentions that the CXL.cachemem protocol errors
detected by a CXL root port could be logged in RCEC AER Extended
Capability. The recommendation solution from CXL r3.1 section 9.18.1.5
is:

	"Probe all CXL Downstream Ports and determine whether they have
	logged an error in the CXL.io or CXL.cachemem status registers."

The new helper function called pcie_walk_rcec_all(), CXL RAS error
handler can use it to locate all CXL root ports or CXL devices in RCEC
associated bus range.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/pci/pci.h       |  6 ++++++
 drivers/pci/pcie/rcec.c | 44 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5ecbcf041179..a068f2d7dd28 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -444,6 +444,9 @@ void pcie_link_rcec(struct pci_dev *rcec);
 void pcie_walk_rcec(struct pci_dev *rcec,
 		    int (*cb)(struct pci_dev *, void *),
 		    void *userdata);
+void pcie_walk_rcec_all(struct pci_dev *rcec,
+			int (*cb)(struct pci_dev *, void *),
+			void *userdata);
 #else
 static inline void pci_rcec_init(struct pci_dev *dev) { }
 static inline void pci_rcec_exit(struct pci_dev *dev) { }
@@ -451,6 +454,9 @@ static inline void pcie_link_rcec(struct pci_dev *rcec) { }
 static inline void pcie_walk_rcec(struct pci_dev *rcec,
 				  int (*cb)(struct pci_dev *, void *),
 				  void *userdata) { }
+static inline void pcie_walk_rcec_all(struct pci_dev *rcec,
+				      int (*cb)(struct pci_dev *, void *),
+				      void *userdata) { }
 #endif
 
 #ifdef CONFIG_PCI_ATS
diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
index d0bcd141ac9c..189de280660c 100644
--- a/drivers/pci/pcie/rcec.c
+++ b/drivers/pci/pcie/rcec.c
@@ -65,6 +65,15 @@ static int walk_rcec_helper(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+static int walk_rcec_all_helper(struct pci_dev *dev, void *data)
+{
+	struct walk_rcec_data *rcec_data = data;
+
+	rcec_data->user_callback(dev, rcec_data->user_data);
+
+	return 0;
+}
+
 static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
 		      void *userdata)
 {
@@ -83,7 +92,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
 	nextbusn = rcec->rcec_ea->nextbusn;
 	lastbusn = rcec->rcec_ea->lastbusn;
 
-	/* All RCiEP devices are on the same bus as the RCEC */
+	/* All devices are on the same bus as the RCEC */
 	if (nextbusn == 0xff && lastbusn == 0x00)
 		return;
 
@@ -96,7 +105,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
 		if (!bus)
 			continue;
 
-		/* Find RCiEP devices on the given bus ranges */
+		/* Find devices on the given bus ranges */
 		pci_walk_bus(bus, cb, rcec_data);
 	}
 }
@@ -146,6 +155,37 @@ void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
 	walk_rcec(walk_rcec_helper, &rcec_data);
 }
 
+/**
+ * pcie_walk_rcec_all - Walk all devices in RCEC's bus range.
+ * @rcec:	RCEC whose devices should be walked
+ * @cb:		Callback to be called for each device found
+ * @userdata:	Arbitrary pointer to be passed to callback
+ *
+ * It is implemented only for CXL cases.
+ * Per CXL r3.1 section 12.2.2, CXL protocol errors detected by
+ * CXL root port could be logged in an RCEC's AER Extended Capability.
+ * And per CXL r3.1 section 9.18.1.5, the recommendation is that probing
+ * all CXL root ports to determine whether they have logged an error.
+ * So provide this function for CXL to walk the given RCEC, CXL driver
+ * will figure out which CXL root ports detected errors.
+ *
+ * If @cb returns anything other than 0, break out.
+ */
+void pcie_walk_rcec_all(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
+			void *userdata)
+{
+	struct walk_rcec_data rcec_data;
+
+	if (!rcec->rcec_ea)
+		return;
+
+	rcec_data.rcec = rcec;
+	rcec_data.user_callback = cb;
+	rcec_data.user_data = userdata;
+
+	walk_rcec(walk_rcec_all_helper, &rcec_data);
+}
+
 void pci_rcec_init(struct pci_dev *dev)
 {
 	struct rcec_ea *rcec_ea;
-- 
2.40.1

Re: [RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Terry Bowman]

Hi Li,

I added comments below.

On 3/13/24 03:35, Li Ming wrote:
> PCIe RCEC core only provides pcie_walk_rcec() to walk all RCiEP devices
> associating with RCEC, but CXL subsystem needs a helper function which
> can walk all devices in RCEC associated bus range other than RCiEPs for
> below RAS error case.
> 
> CXL r3.1 section 12.2.2 mentions that the CXL.cachemem protocol errors
> detected by a CXL root port could be logged in RCEC AER Extended
> Capability. The recommendation solution from CXL r3.1 section 9.18.1.5
> is:
> 
> 	"Probe all CXL Downstream Ports and determine whether they have
> 	logged an error in the CXL.io or CXL.cachemem status registers."
> 
> The new helper function called pcie_walk_rcec_all(), CXL RAS error
> handler can use it to locate all CXL root ports or CXL devices in RCEC
> associated bus range.

The RCEC-root port relation you mention is new to me. Typically, not in 
all cases, RCH-RCD has a RCEC. And a VH mode system has a root port 
instead. The RCH RCEC and VH root port are both bound to the PCIeport 
bus driver that supports handling and logging AER. This allows the PCIe 
port bus driver to handle AER in a RCEC and root port AER using the same 
procedure and accesses to the AER capability registers. 

This is oversimplified but are you looking to handle root port AER error 
in the RCEC from the below diagram? 

RCEC <--> CXL root port (bridge) <--> Endpoint

> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/pci/pci.h       |  6 ++++++
>  drivers/pci/pcie/rcec.c | 44 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5ecbcf041179..a068f2d7dd28 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -444,6 +444,9 @@ void pcie_link_rcec(struct pci_dev *rcec);
>  void pcie_walk_rcec(struct pci_dev *rcec,
>  		    int (*cb)(struct pci_dev *, void *),
>  		    void *userdata);
> +void pcie_walk_rcec_all(struct pci_dev *rcec,
> +			int (*cb)(struct pci_dev *, void *),
> +			void *userdata);
>  #else
>  static inline void pci_rcec_init(struct pci_dev *dev) { }
>  static inline void pci_rcec_exit(struct pci_dev *dev) { }
> @@ -451,6 +454,9 @@ static inline void pcie_link_rcec(struct pci_dev *rcec) { }
>  static inline void pcie_walk_rcec(struct pci_dev *rcec,
>  				  int (*cb)(struct pci_dev *, void *),
>  				  void *userdata) { }
> +static inline void pcie_walk_rcec_all(struct pci_dev *rcec,
> +				      int (*cb)(struct pci_dev *, void *),
> +				      void *userdata) { }
>  #endif
>  
>  #ifdef CONFIG_PCI_ATS
> diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
> index d0bcd141ac9c..189de280660c 100644
> --- a/drivers/pci/pcie/rcec.c
> +++ b/drivers/pci/pcie/rcec.c
> @@ -65,6 +65,15 @@ static int walk_rcec_helper(struct pci_dev *dev, void *data)
>  	return 0;
>  }
>  
> +static int walk_rcec_all_helper(struct pci_dev *dev, void *data)
> +{
> +	struct walk_rcec_data *rcec_data = data;
> +
> +	rcec_data->user_callback(dev, rcec_data->user_data);
> +
> +	return 0;
> +}
> +
>  static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>  		      void *userdata)
>  {
> @@ -83,7 +92,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>  	nextbusn = rcec->rcec_ea->nextbusn;
>  	lastbusn = rcec->rcec_ea->lastbusn;
>  
> -	/* All RCiEP devices are on the same bus as the RCEC */
> +	/* All devices are on the same bus as the RCEC */

RCiEPs are not guaranteed to be on same bus as RCEC. Details for associated 
next and last busses:

"This register does not indicate association between an Event Collector and 
any Function on the same Bus Number as the Event Collector itself, however 
it is permitted for the Association Bus Range to include the Bus Number of 
the Root Complex Event Collector."[1]

[1] PCI Spec 6.0 - RCEC Associated Bus Numbers Register 9Ofset 08h)


>  	if (nextbusn == 0xff && lastbusn == 0x00)
>  		return;
>  
> @@ -96,7 +105,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>  		if (!bus)
>  			continue;
>  
> -		/* Find RCiEP devices on the given bus ranges */
> +		/* Find devices on the given bus ranges */
>  		pci_walk_bus(bus, cb, rcec_data);
>  	}
>  }
> @@ -146,6 +155,37 @@ void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
>  	walk_rcec(walk_rcec_helper, &rcec_data);
>  }
>  
> +/**
> + * pcie_walk_rcec_all - Walk all devices in RCEC's bus range.
> + * @rcec:	RCEC whose devices should be walked
> + * @cb:		Callback to be called for each device found
> + * @userdata:	Arbitrary pointer to be passed to callback
> + *
> + * It is implemented only for CXL cases.
> + * Per CXL r3.1 section 12.2.2, CXL protocol errors detected by
> + * CXL root port could be logged in an RCEC's AER Extended Capability.
> + * And per CXL r3.1 section 9.18.1.5, the recommendation is that probing
> + * all CXL root ports to determine whether they have logged an error.
> + * So provide this function for CXL to walk the given RCEC, CXL driver
> + * will figure out which CXL root ports detected errors.
> + *
> + * If @cb returns anything other than 0, break out.
> + */
> +void pcie_walk_rcec_all(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> +			void *userdata)
> +{

Is this identical to pcie_walk_rcec()? Is this addition necessary?

Regards,
Terry

> +	struct walk_rcec_data rcec_data;
> +
> +	if (!rcec->rcec_ea)
> +		return;
> +
> +	rcec_data.rcec = rcec;
> +	rcec_data.user_callback = cb;
> +	rcec_data.user_data = userdata;
> +
> +	walk_rcec(walk_rcec_all_helper, &rcec_data);
> +}
> +
>  void pci_rcec_init(struct pci_dev *dev)
>  {
>  	struct rcec_ea *rcec_ea;

Re: [RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Dan Williams]

Terry Bowman wrote:
> Hi Li,
> 
> I added comments below.
> 
> On 3/13/24 03:35, Li Ming wrote:
> > PCIe RCEC core only provides pcie_walk_rcec() to walk all RCiEP devices
> > associating with RCEC, but CXL subsystem needs a helper function which
> > can walk all devices in RCEC associated bus range other than RCiEPs for
> > below RAS error case.
> > 
> > CXL r3.1 section 12.2.2 mentions that the CXL.cachemem protocol errors
> > detected by a CXL root port could be logged in RCEC AER Extended
> > Capability. The recommendation solution from CXL r3.1 section 9.18.1.5
> > is:
> > 
> > 	"Probe all CXL Downstream Ports and determine whether they have
> > 	logged an error in the CXL.io or CXL.cachemem status registers."
> > 
> > The new helper function called pcie_walk_rcec_all(), CXL RAS error
> > handler can use it to locate all CXL root ports or CXL devices in RCEC
> > associated bus range.
> 
> The RCEC-root port relation you mention is new to me. Typically, not in 
> all cases, RCH-RCD has a RCEC. And a VH mode system has a root port 
> instead. The RCH RCEC and VH root port are both bound to the PCIeport 
> bus driver that supports handling and logging AER. This allows the PCIe 
> port bus driver to handle AER in a RCEC and root port AER using the same 
> procedure and accesses to the AER capability registers. 
> 
> This is oversimplified but are you looking to handle root port AER error 
> in the RCEC from the below diagram? 
> 
> RCEC <--> CXL root port (bridge) <--> Endpoint
> 
> > 
> > Signed-off-by: Li Ming <ming4.li@intel.com>
> > ---
> >  drivers/pci/pci.h       |  6 ++++++
> >  drivers/pci/pcie/rcec.c | 44 +++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 48 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 5ecbcf041179..a068f2d7dd28 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -444,6 +444,9 @@ void pcie_link_rcec(struct pci_dev *rcec);
> >  void pcie_walk_rcec(struct pci_dev *rcec,
> >  		    int (*cb)(struct pci_dev *, void *),
> >  		    void *userdata);
> > +void pcie_walk_rcec_all(struct pci_dev *rcec,
> > +			int (*cb)(struct pci_dev *, void *),
> > +			void *userdata);
> >  #else
> >  static inline void pci_rcec_init(struct pci_dev *dev) { }
> >  static inline void pci_rcec_exit(struct pci_dev *dev) { }
> > @@ -451,6 +454,9 @@ static inline void pcie_link_rcec(struct pci_dev *rcec) { }
> >  static inline void pcie_walk_rcec(struct pci_dev *rcec,
> >  				  int (*cb)(struct pci_dev *, void *),
> >  				  void *userdata) { }
> > +static inline void pcie_walk_rcec_all(struct pci_dev *rcec,
> > +				      int (*cb)(struct pci_dev *, void *),
> > +				      void *userdata) { }
> >  #endif
> >  
> >  #ifdef CONFIG_PCI_ATS
> > diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
> > index d0bcd141ac9c..189de280660c 100644
> > --- a/drivers/pci/pcie/rcec.c
> > +++ b/drivers/pci/pcie/rcec.c
> > @@ -65,6 +65,15 @@ static int walk_rcec_helper(struct pci_dev *dev, void *data)
> >  	return 0;
> >  }
> >  
> > +static int walk_rcec_all_helper(struct pci_dev *dev, void *data)
> > +{
> > +	struct walk_rcec_data *rcec_data = data;
> > +
> > +	rcec_data->user_callback(dev, rcec_data->user_data);
> > +
> > +	return 0;
> > +}
> > +
> >  static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
> >  		      void *userdata)
> >  {
> > @@ -83,7 +92,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
> >  	nextbusn = rcec->rcec_ea->nextbusn;
> >  	lastbusn = rcec->rcec_ea->lastbusn;
> >  
> > -	/* All RCiEP devices are on the same bus as the RCEC */
> > +	/* All devices are on the same bus as the RCEC */
> 
> RCiEPs are not guaranteed to be on same bus as RCEC. Details for associated 
> next and last busses:
> 
> "This register does not indicate association between an Event Collector and 
> any Function on the same Bus Number as the Event Collector itself, however 
> it is permitted for the Association Bus Range to include the Bus Number of 
> the Root Complex Event Collector."[1]
> 
> [1] PCI Spec 6.0 - RCEC Associated Bus Numbers Register 9Ofset 08h)

Hi Terry,

This patchset is responding to the implications of the implementation
note in 9.18.1.5 RCEC Downstream Port Association Structure (RDPAS).
That says that CXL.io and CXL.cachemem errors in Root Ports may indeed
be signaled to an RCEC. Do you expect that implementation note to cause
any issues on platforms that do not follow that CXL spec behavior?

My expectation is that it may just cause extra polling for errors, but
not cause any harm.

Re: [RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Terry Bowman]

Hi Dan,

On 4/15/24 23:39, Dan Williams wrote:
> Terry Bowman wrote:
>> Hi Li,
>>
>> I added comments below.
>>
>> On 3/13/24 03:35, Li Ming wrote:
>>> PCIe RCEC core only provides pcie_walk_rcec() to walk all RCiEP devices
>>> associating with RCEC, but CXL subsystem needs a helper function which
>>> can walk all devices in RCEC associated bus range other than RCiEPs for
>>> below RAS error case.
>>>
>>> CXL r3.1 section 12.2.2 mentions that the CXL.cachemem protocol errors
>>> detected by a CXL root port could be logged in RCEC AER Extended
>>> Capability. The recommendation solution from CXL r3.1 section 9.18.1.5
>>> is:
>>>
>>> 	"Probe all CXL Downstream Ports and determine whether they have
>>> 	logged an error in the CXL.io or CXL.cachemem status registers."
>>>
>>> The new helper function called pcie_walk_rcec_all(), CXL RAS error
>>> handler can use it to locate all CXL root ports or CXL devices in RCEC
>>> associated bus range.
>>
>> The RCEC-root port relation you mention is new to me. Typically, not in 
>> all cases, RCH-RCD has a RCEC. And a VH mode system has a root port 
>> instead. The RCH RCEC and VH root port are both bound to the PCIeport 
>> bus driver that supports handling and logging AER. This allows the PCIe 
>> port bus driver to handle AER in a RCEC and root port AER using the same 
>> procedure and accesses to the AER capability registers. 
>>
>> This is oversimplified but are you looking to handle root port AER error 
>> in the RCEC from the below diagram? 
>>
>> RCEC <--> CXL root port (bridge) <--> Endpoint
>>
>>>
>>> Signed-off-by: Li Ming <ming4.li@intel.com>
>>> ---
>>>  drivers/pci/pci.h       |  6 ++++++
>>>  drivers/pci/pcie/rcec.c | 44 +++++++++++++++++++++++++++++++++++++++--
>>>  2 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>> index 5ecbcf041179..a068f2d7dd28 100644
>>> --- a/drivers/pci/pci.h
>>> +++ b/drivers/pci/pci.h
>>> @@ -444,6 +444,9 @@ void pcie_link_rcec(struct pci_dev *rcec);
>>>  void pcie_walk_rcec(struct pci_dev *rcec,
>>>  		    int (*cb)(struct pci_dev *, void *),
>>>  		    void *userdata);
>>> +void pcie_walk_rcec_all(struct pci_dev *rcec,
>>> +			int (*cb)(struct pci_dev *, void *),
>>> +			void *userdata);
>>>  #else
>>>  static inline void pci_rcec_init(struct pci_dev *dev) { }
>>>  static inline void pci_rcec_exit(struct pci_dev *dev) { }
>>> @@ -451,6 +454,9 @@ static inline void pcie_link_rcec(struct pci_dev *rcec) { }
>>>  static inline void pcie_walk_rcec(struct pci_dev *rcec,
>>>  				  int (*cb)(struct pci_dev *, void *),
>>>  				  void *userdata) { }
>>> +static inline void pcie_walk_rcec_all(struct pci_dev *rcec,
>>> +				      int (*cb)(struct pci_dev *, void *),
>>> +				      void *userdata) { }
>>>  #endif
>>>  
>>>  #ifdef CONFIG_PCI_ATS
>>> diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
>>> index d0bcd141ac9c..189de280660c 100644
>>> --- a/drivers/pci/pcie/rcec.c
>>> +++ b/drivers/pci/pcie/rcec.c
>>> @@ -65,6 +65,15 @@ static int walk_rcec_helper(struct pci_dev *dev, void *data)
>>>  	return 0;
>>>  }
>>>  
>>> +static int walk_rcec_all_helper(struct pci_dev *dev, void *data)
>>> +{
>>> +	struct walk_rcec_data *rcec_data = data;
>>> +
>>> +	rcec_data->user_callback(dev, rcec_data->user_data);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>>>  		      void *userdata)
>>>  {
>>> @@ -83,7 +92,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>>>  	nextbusn = rcec->rcec_ea->nextbusn;
>>>  	lastbusn = rcec->rcec_ea->lastbusn;
>>>  
>>> -	/* All RCiEP devices are on the same bus as the RCEC */
>>> +	/* All devices are on the same bus as the RCEC */
>>
>> RCiEPs are not guaranteed to be on same bus as RCEC. Details for associated 
>> next and last busses:
>>
>> "This register does not indicate association between an Event Collector and 
>> any Function on the same Bus Number as the Event Collector itself, however 
>> it is permitted for the Association Bus Range to include the Bus Number of 
>> the Root Complex Event Collector."[1]
>>
>> [1] PCI Spec 6.0 - RCEC Associated Bus Numbers Register 9Ofset 08h)
> 
> Hi Terry,
> 
> This patchset is responding to the implications of the implementation
> note in 9.18.1.5 RCEC Downstream Port Association Structure (RDPAS).
> That says that CXL.io and CXL.cachemem errors in Root Ports may indeed
> be signaled to an RCEC. Do you expect that implementation note to cause
> any issues on platforms that do not follow that CXL spec behavior?
> 
> My expectation is that it may just cause extra polling for errors, but
> not cause any harm.

AMD platforms in RCH/RCD mode consume protocol errors in the RCEC's AER driver. AMD 
platforms in VH mode consume protocol errors (including root port errors) in the 
root port's AER driver. The exception is the VH mode host with CXL1.1 endpoint and 
RCH downstream errors. CXL1.1 endpoint and RCH downstream errors in a VH host are 
consumed in the RCEC.

I don't believe these patchset changes would affect this behavior. But, I will need 
to test to confirm.

Regards,
Terry

Re: [RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Dan Williams]

Terry Bowman wrote:
[..]
> > Hi Terry,
> > 
> > This patchset is responding to the implications of the implementation
> > note in 9.18.1.5 RCEC Downstream Port Association Structure (RDPAS).
> > That says that CXL.io and CXL.cachemem errors in Root Ports may indeed
> > be signaled to an RCEC. Do you expect that implementation note to cause
> > any issues on platforms that do not follow that CXL spec behavior?
> > 
> > My expectation is that it may just cause extra polling for errors, but
> > not cause any harm.
> 
> AMD platforms in RCH/RCD mode consume protocol errors in the RCEC's AER driver. AMD 
> platforms in VH mode consume protocol errors (including root port errors) in the 
> root port's AER driver. The exception is the VH mode host with CXL1.1 endpoint and 
> RCH downstream errors. CXL1.1 endpoint and RCH downstream errors in a VH host are 
> consumed in the RCEC.

I agree that's the most compatible path for existing software.

> I don't believe these patchset changes would affect this behavior. But, I will need 
> to test to confirm.

As I wrote to Li Ming, I think any potential conflict can further be
limited by the fact that this extra scanning is limited to CXL.cachemem,
not typical PCI AER flows.

Re: [RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Li, Ming]

On 4/23/2024 7:03 AM, Dan Williams wrote:
> Terry Bowman wrote:
> [..]
>>> Hi Terry,
>>>
>>> This patchset is responding to the implications of the implementation
>>> note in 9.18.1.5 RCEC Downstream Port Association Structure (RDPAS).
>>> That says that CXL.io and CXL.cachemem errors in Root Ports may indeed
>>> be signaled to an RCEC. Do you expect that implementation note to cause
>>> any issues on platforms that do not follow that CXL spec behavior?
>>>
>>> My expectation is that it may just cause extra polling for errors, but
>>> not cause any harm.
>>
>> AMD platforms in RCH/RCD mode consume protocol errors in the RCEC's AER driver. AMD 
>> platforms in VH mode consume protocol errors (including root port errors) in the 
>> root port's AER driver. The exception is the VH mode host with CXL1.1 endpoint and 
>> RCH downstream errors. CXL1.1 endpoint and RCH downstream errors in a VH host are 
>> consumed in the RCEC.
> 
> I agree that's the most compatible path for existing software.
> 
>> I don't believe these patchset changes would affect this behavior. But, I will need 
>> to test to confirm.
> 
> As I wrote to Li Ming, I think any potential conflict can further be
> limited by the fact that this extra scanning is limited to CXL.cachemem,
> not typical PCI AER flows.

Agree with Dan, but I think that software does not have a chance to know if the error is a CXL.cachemem error withour RDPAS(only knows it is a uncor_internal_error/cor_internal_error reported by RCEC), maybe we can limit this extra scanning for the RPs working on CXL mode?

Re: [RFC PATCH 1/6] PCI/RCEC: Introduce pcie_walk_rcec_all()

[Li, Ming]

On 3/26/2024 4:15 AM, Terry Bowman wrote:
> Hi Li,
> 
> I added comments below.
> 
> On 3/13/24 03:35, Li Ming wrote:
>> PCIe RCEC core only provides pcie_walk_rcec() to walk all RCiEP devices
>> associating with RCEC, but CXL subsystem needs a helper function which
>> can walk all devices in RCEC associated bus range other than RCiEPs for
>> below RAS error case.
>>
>> CXL r3.1 section 12.2.2 mentions that the CXL.cachemem protocol errors
>> detected by a CXL root port could be logged in RCEC AER Extended
>> Capability. The recommendation solution from CXL r3.1 section 9.18.1.5
>> is:
>>
>> 	"Probe all CXL Downstream Ports and determine whether they have
>> 	logged an error in the CXL.io or CXL.cachemem status registers."
>>
>> The new helper function called pcie_walk_rcec_all(), CXL RAS error
>> handler can use it to locate all CXL root ports or CXL devices in RCEC
>> associated bus range.
> 
> The RCEC-root port relation you mention is new to me. Typically, not in 
> all cases, RCH-RCD has a RCEC. And a VH mode system has a root port 
> instead. The RCH RCEC and VH root port are both bound to the PCIeport 
> bus driver that supports handling and logging AER. This allows the PCIe 
> port bus driver to handle AER in a RCEC and root port AER using the same 
> procedure and accesses to the AER capability registers. 
> 
> This is oversimplified but are you looking to handle root port AER error 
> in the RCEC from the below diagram? 
> 
> RCEC <--> CXL root port (bridge) <--> Endpoint

Hi Terry,

Thank you for your comments.
Yes, if CXL root port detected a CXL.cachemem protocol error, the error is possible to be logged in RCEC AER Extended Capability as Uncorrectable Internal Error or Correctable Internal Error.
But the cxl.cachemem protocol error is logged in the CXL root port RAS capability at the same time. So the implementation is not only handle Uncorrectable Internal Error/Correctable Internal Error on RCEC AER capability, but also find out the CXL root port reported this error and handle the RAS error in the CXL root port.



> 
>>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>>  drivers/pci/pci.h       |  6 ++++++
>>  drivers/pci/pcie/rcec.c | 44 +++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 5ecbcf041179..a068f2d7dd28 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -444,6 +444,9 @@ void pcie_link_rcec(struct pci_dev *rcec);
>>  void pcie_walk_rcec(struct pci_dev *rcec,
>>  		    int (*cb)(struct pci_dev *, void *),
>>  		    void *userdata);
>> +void pcie_walk_rcec_all(struct pci_dev *rcec,
>> +			int (*cb)(struct pci_dev *, void *),
>> +			void *userdata);
>>  #else
>>  static inline void pci_rcec_init(struct pci_dev *dev) { }
>>  static inline void pci_rcec_exit(struct pci_dev *dev) { }
>> @@ -451,6 +454,9 @@ static inline void pcie_link_rcec(struct pci_dev *rcec) { }
>>  static inline void pcie_walk_rcec(struct pci_dev *rcec,
>>  				  int (*cb)(struct pci_dev *, void *),
>>  				  void *userdata) { }
>> +static inline void pcie_walk_rcec_all(struct pci_dev *rcec,
>> +				      int (*cb)(struct pci_dev *, void *),
>> +				      void *userdata) { }
>>  #endif
>>  
>>  #ifdef CONFIG_PCI_ATS
>> diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
>> index d0bcd141ac9c..189de280660c 100644
>> --- a/drivers/pci/pcie/rcec.c
>> +++ b/drivers/pci/pcie/rcec.c
>> @@ -65,6 +65,15 @@ static int walk_rcec_helper(struct pci_dev *dev, void *data)
>>  	return 0;
>>  }
>>  
>> +static int walk_rcec_all_helper(struct pci_dev *dev, void *data)
>> +{
>> +	struct walk_rcec_data *rcec_data = data;
>> +
>> +	rcec_data->user_callback(dev, rcec_data->user_data);
>> +
>> +	return 0;
>> +}
>> +
>>  static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>>  		      void *userdata)
>>  {
>> @@ -83,7 +92,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>>  	nextbusn = rcec->rcec_ea->nextbusn;
>>  	lastbusn = rcec->rcec_ea->lastbusn;
>>  
>> -	/* All RCiEP devices are on the same bus as the RCEC */
>> +	/* All devices are on the same bus as the RCEC */
> 
> RCiEPs are not guaranteed to be on same bus as RCEC. Details for associated 
> next and last busses:
> 
> "This register does not indicate association between an Event Collector and 
> any Function on the same Bus Number as the Event Collector itself, however 
> it is permitted for the Association Bus Range to include the Bus Number of 
> the Root Complex Event Collector."[1]

I agree with you, the comment in code makes confusion, actually, walk_rcec() already walks the bus same as RCEC's and the bus range defined in RCEC Associated Bus Number Register.

> 
> [1] PCI Spec 6.0 - RCEC Associated Bus Numbers Register 9Ofset 08h)
> 
> 
>>  	if (nextbusn == 0xff && lastbusn == 0x00)
>>  		return;
>>  
>> @@ -96,7 +105,7 @@ static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
>>  		if (!bus)
>>  			continue;
>>  
>> -		/* Find RCiEP devices on the given bus ranges */
>> +		/* Find devices on the given bus ranges */
>>  		pci_walk_bus(bus, cb, rcec_data);
>>  	}
>>  }
>> @@ -146,6 +155,37 @@ void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
>>  	walk_rcec(walk_rcec_helper, &rcec_data);
>>  }
>>  
>> +/**
>> + * pcie_walk_rcec_all - Walk all devices in RCEC's bus range.
>> + * @rcec:	RCEC whose devices should be walked
>> + * @cb:		Callback to be called for each device found
>> + * @userdata:	Arbitrary pointer to be passed to callback
>> + *
>> + * It is implemented only for CXL cases.
>> + * Per CXL r3.1 section 12.2.2, CXL protocol errors detected by
>> + * CXL root port could be logged in an RCEC's AER Extended Capability.
>> + * And per CXL r3.1 section 9.18.1.5, the recommendation is that probing
>> + * all CXL root ports to determine whether they have logged an error.
>> + * So provide this function for CXL to walk the given RCEC, CXL driver
>> + * will figure out which CXL root ports detected errors.
>> + *
>> + * If @cb returns anything other than 0, break out.
>> + */
>> +void pcie_walk_rcec_all(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
>> +			void *userdata)
>> +{
> 
> Is this identical to pcie_walk_rcec()? Is this addition necessary?
> 

pcie_walk_rcec() only walks all RCiEPs in the RCEC associated bus range, 
My implementation needs to walk all cxl root ports in the RCEC associated bus range to handle CXL root port error logged in RCEC AER Extended Capability case. 

Thanks
Ming

> Regards,
> Terry
> 
>> +	struct walk_rcec_data rcec_data;
>> +
>> +	if (!rcec->rcec_ea)
>> +		return;
>> +
>> +	rcec_data.rcec = rcec;
>> +	rcec_data.user_callback = cb;
>> +	rcec_data.user_data = userdata;
>> +
>> +	walk_rcec(walk_rcec_all_helper, &rcec_data);
>> +}
>> +
>>  void pci_rcec_init(struct pci_dev *dev)
>>  {
>>  	struct rcec_ea *rcec_ea;

[RFC PATCH 2/6] PCI/CXL: A new attribute to indicate CXL-capable host bridge

[Li Ming]

Introduce a new attribute called 'is_cxl' in struct pci_host_bridge to
indicate if the PCI host bridge is CXl capable.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/acpi/pci_root.c | 1 +
 include/linux/pci.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 58b89b8d950e..4ab0dc8450ce 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1034,6 +1034,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		goto out_release_info;
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
+	host_bridge->is_cxl = is_cxl(root);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
 		host_bridge->native_pcie_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
diff --git a/include/linux/pci.h b/include/linux/pci.h
index bf6c02bee49f..bbe90e730285 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -587,6 +587,7 @@ struct pci_host_bridge {
 	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
 	unsigned int	size_windows:1;		/* Enable root bus sizing */
 	unsigned int	msi_domain:1;		/* Bridge wants MSI domain */
+	unsigned int	is_cxl:1;		/* CXL capable Host Bridge */
 
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
-- 
2.40.1

[RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Li Ming]

Per CXl r3.1 section 12.2.2, CXL.cachemem protocol erros detected by CXL
root port could be logged in RCEC AER Extended Capability as
PCI_ERR_UNC_INTN or PCI_ERR_COR_INTERNAL. Unmask these errors for that
case.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/pci/pcie/aer.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 42a3bd35a3e1..364c74e47273 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -985,7 +985,7 @@ static bool cxl_error_is_native(struct pci_dev *dev)
 {
 	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
 
-	return (pcie_ports_native || host->native_aer);
+	return (pcie_ports_native || host->native_aer) && host->is_cxl;
 }
 
 static bool is_internal_error(struct aer_err_info *info)
@@ -1041,8 +1041,13 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
 {
 	bool *handles_cxl = data;
 
-	if (!*handles_cxl)
-		*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
+	if (!*handles_cxl && cxl_error_is_native(dev)) {
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END &&
+		    dev->rcec && is_cxl_mem_dev(dev))
+			*handles_cxl = true;
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
+			*handles_cxl = true;
+	}
 
 	/* Non-zero terminates iteration */
 	return *handles_cxl;
@@ -1054,13 +1059,18 @@ static bool handles_cxl_errors(struct pci_dev *rcec)
 
 	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
 	    pcie_aer_is_native(rcec))
-		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
+		pcie_walk_rcec_all(rcec, handles_cxl_error_iter, &handles_cxl);
 
 	return handles_cxl;
 }
 
-static void cxl_rch_enable_rcec(struct pci_dev *rcec)
+static void cxl_enable_rcec(struct pci_dev *rcec)
 {
+	/*
+	 * Enable RCEC's internal error report for two cases:
+	 * 1. RCiEP detected CXL.cachemem protocol errors
+	 * 2. CXL root port detected CXL.cachemem protocol errors.
+	 */
 	if (!handles_cxl_errors(rcec))
 		return;
 
@@ -1069,7 +1079,7 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
 }
 
 #else
-static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
+static inline void cxl_enable_rcec(struct pci_dev *dev) { }
 static inline void cxl_rch_handle_error(struct pci_dev *dev,
 					struct aer_err_info *info) { }
 #endif
@@ -1494,7 +1504,7 @@ static int aer_probe(struct pcie_device *dev)
 		return status;
 	}
 
-	cxl_rch_enable_rcec(port);
+	cxl_enable_rcec(port);
 	aer_enable_rootport(rpc);
 	pci_info(port, "enabled with IRQ %d\n", dev->irq);
 	return 0;
-- 
2.40.1

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Terry Bowman]

Hi Li, 

I added comments below.

On 3/13/24 03:35, Li Ming wrote:
> Per CXl r3.1 section 12.2.2, CXL.cachemem protocol erros detected by CXL
> root port could be logged in RCEC AER Extended Capability as
> PCI_ERR_UNC_INTN or PCI_ERR_COR_INTERNAL. Unmask these errors for that
> case.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/pci/pcie/aer.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 42a3bd35a3e1..364c74e47273 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -985,7 +985,7 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>  {
>  	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>  
> -	return (pcie_ports_native || host->native_aer);
> +	return (pcie_ports_native || host->native_aer) && host->is_cxl;
>  }
>  
>  static bool is_internal_error(struct aer_err_info *info)
> @@ -1041,8 +1041,13 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>  {
>  	bool *handles_cxl = data;
>  
> -	if (!*handles_cxl)
> -		*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
> +	if (!*handles_cxl && cxl_error_is_native(dev)) {
> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END &&
> +		    dev->rcec && is_cxl_mem_dev(dev))
> +			*handles_cxl = true;
> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> +			*handles_cxl = true;
> +	}
I understand a root port can be found under an RCEC. It's possible. But, does the downstream 
root port forward AER to the upstream RCEC? My understanding is AER is handled and processed
at the first root port/RCEC upstream from the device/RCH/USP/DSP.
 
Regards,
Terry

>  
>  	/* Non-zero terminates iteration */
>  	return *handles_cxl;
> @@ -1054,13 +1059,18 @@ static bool handles_cxl_errors(struct pci_dev *rcec)
>  
>  	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
>  	    pcie_aer_is_native(rcec))
> -		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
> +		pcie_walk_rcec_all(rcec, handles_cxl_error_iter, &handles_cxl);
>  
>  	return handles_cxl;
>  }
>  
> -static void cxl_rch_enable_rcec(struct pci_dev *rcec)
> +static void cxl_enable_rcec(struct pci_dev *rcec)
>  {
> +	/*
> +	 * Enable RCEC's internal error report for two cases:
> +	 * 1. RCiEP detected CXL.cachemem protocol errors
> +	 * 2. CXL root port detected CXL.cachemem protocol errors.
> +	 */
>  	if (!handles_cxl_errors(rcec))
>  		return;
>  
> @@ -1069,7 +1079,7 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>  }
>  
>  #else
> -static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
> +static inline void cxl_enable_rcec(struct pci_dev *dev) { }
>  static inline void cxl_rch_handle_error(struct pci_dev *dev,
>  					struct aer_err_info *info) { }
>  #endif
> @@ -1494,7 +1504,7 @@ static int aer_probe(struct pcie_device *dev)
>  		return status;
>  	}
>  
> -	cxl_rch_enable_rcec(port);
> +	cxl_enable_rcec(port);
>  	aer_enable_rootport(rpc);
>  	pci_info(port, "enabled with IRQ %d\n", dev->irq);
>  	return 0;

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Li, Ming]

On 3/26/2024 3:42 AM, Terry Bowman wrote:
> Hi Li, 
> 
> I added comments below.
> 
> On 3/13/24 03:35, Li Ming wrote:
>> Per CXl r3.1 section 12.2.2, CXL.cachemem protocol erros detected by CXL
>> root port could be logged in RCEC AER Extended Capability as
>> PCI_ERR_UNC_INTN or PCI_ERR_COR_INTERNAL. Unmask these errors for that
>> case.
>>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>>  drivers/pci/pcie/aer.c | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 42a3bd35a3e1..364c74e47273 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -985,7 +985,7 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>>  {
>>  	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>>  
>> -	return (pcie_ports_native || host->native_aer);
>> +	return (pcie_ports_native || host->native_aer) && host->is_cxl;
>>  }
>>  
>>  static bool is_internal_error(struct aer_err_info *info)
>> @@ -1041,8 +1041,13 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>  {
>>  	bool *handles_cxl = data;
>>  
>> -	if (!*handles_cxl)
>> -		*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
>> +	if (!*handles_cxl && cxl_error_is_native(dev)) {
>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END &&
>> +		    dev->rcec && is_cxl_mem_dev(dev))
>> +			*handles_cxl = true;
>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>> +			*handles_cxl = true;
>> +	}
> I understand a root port can be found under an RCEC. It's possible. But, does the downstream 
> root port forward AER to the upstream RCEC? My understanding is AER is handled and processed
> at the first root port/RCEC upstream from the device/RCH/USP/DSP.
>  
> Regards,
> Terry
> 

CXL r3.1 section 12.2.2 mentions this:

"If the CXL.cachemem protocol errors detected by a CXL root port are logged as
CIEs or UIEs in an RCEC’s AER Extended Capability, it is recommended that the System
Firmware populate an RDPAS record (see Section 9.18.1.5) to establish the association
between the RCEC and the root port."

I think it means that CXL root port is possible to forward its AER to RCEC.

Thanks
Ming

>>  
>>  	/* Non-zero terminates iteration */
>>  	return *handles_cxl;
>> @@ -1054,13 +1059,18 @@ static bool handles_cxl_errors(struct pci_dev *rcec)
>>  
>>  	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
>>  	    pcie_aer_is_native(rcec))
>> -		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
>> +		pcie_walk_rcec_all(rcec, handles_cxl_error_iter, &handles_cxl);
>>  
>>  	return handles_cxl;
>>  }
>>  
>> -static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>> +static void cxl_enable_rcec(struct pci_dev *rcec)
>>  {
>> +	/*
>> +	 * Enable RCEC's internal error report for two cases:
>> +	 * 1. RCiEP detected CXL.cachemem protocol errors
>> +	 * 2. CXL root port detected CXL.cachemem protocol errors.
>> +	 */
>>  	if (!handles_cxl_errors(rcec))
>>  		return;
>>  
>> @@ -1069,7 +1079,7 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>  }
>>  
>>  #else
>> -static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
>> +static inline void cxl_enable_rcec(struct pci_dev *dev) { }
>>  static inline void cxl_rch_handle_error(struct pci_dev *dev,
>>  					struct aer_err_info *info) { }
>>  #endif
>> @@ -1494,7 +1504,7 @@ static int aer_probe(struct pcie_device *dev)
>>  		return status;
>>  	}
>>  
>> -	cxl_rch_enable_rcec(port);
>> +	cxl_enable_rcec(port);
>>  	aer_enable_rootport(rpc);
>>  	pci_info(port, "enabled with IRQ %d\n", dev->irq);
>>  	return 0;

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Terry Bowman]

Hi Ming,

On 4/16/24 02:27, Li, Ming wrote:
> On 3/26/2024 3:42 AM, Terry Bowman wrote:
>> Hi Li, 
>>
>> I added comments below.
>>
>> On 3/13/24 03:35, Li Ming wrote:
>>> Per CXl r3.1 section 12.2.2, CXL.cachemem protocol erros detected by CXL
>>> root port could be logged in RCEC AER Extended Capability as
>>> PCI_ERR_UNC_INTN or PCI_ERR_COR_INTERNAL. Unmask these errors for that
>>> case.
>>>
>>> Signed-off-by: Li Ming <ming4.li@intel.com>
>>> ---
>>>  drivers/pci/pcie/aer.c | 24 +++++++++++++++++-------
>>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>> index 42a3bd35a3e1..364c74e47273 100644
>>> --- a/drivers/pci/pcie/aer.c
>>> +++ b/drivers/pci/pcie/aer.c
>>> @@ -985,7 +985,7 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>>>  {
>>>  	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>>>  
>>> -	return (pcie_ports_native || host->native_aer);
>>> +	return (pcie_ports_native || host->native_aer) && host->is_cxl;
>>>  }
>>>  
>>>  static bool is_internal_error(struct aer_err_info *info)
>>> @@ -1041,8 +1041,13 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>>  {
>>>  	bool *handles_cxl = data;
>>>  
>>> -	if (!*handles_cxl)
>>> -		*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
>>> +	if (!*handles_cxl && cxl_error_is_native(dev)) {
>>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END &&
>>> +		    dev->rcec && is_cxl_mem_dev(dev))
>>> +			*handles_cxl = true;
>>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>>> +			*handles_cxl = true;
>>> +	}
>> I understand a root port can be found under an RCEC. It's possible. But, does the downstream 
>> root port forward AER to the upstream RCEC? My understanding is AER is handled and processed
>> at the first root port/RCEC upstream from the device/RCH/USP/DSP.
>>  
>> Regards,
>> Terry
>>
> 
> CXL r3.1 section 12.2.2 mentions this:
> 
> "If the CXL.cachemem protocol errors detected by a CXL root port are logged as
> CIEs or UIEs in an RCEC’s AER Extended Capability, it is recommended that the System
> Firmware populate an RDPAS record (see Section 9.18.1.5) to establish the association
> between the RCEC and the root port."
> 
> I think it means that CXL root port is possible to forward its AER to RCEC.
> 
> Thanks
> Ming
> 

Thanks for pointing to spec details. 

In testing here, we used root port as agent to consume root port CXL protocol errors.
The logic to handle the root port errors requires little to no AER driver changes.
This results in a root port consuming VH protocol errors and RCEC consuming RCD 
protocol errors. The RCEC and root port both use the PCIe port bus driver's AER service
driver in separate instances for RCEC-RCD and root-port-VH.

The driver support is much simpler if RCEC does not handle VH protocol errors. Is there 
a reason to forward root port VH mode protocol errors to an RCEC rather than consume 
in the root port's AER driver and forward to CXL error handler? 

Regards,
Terry

>>>  
>>>  	/* Non-zero terminates iteration */
>>>  	return *handles_cxl;
>>> @@ -1054,13 +1059,18 @@ static bool handles_cxl_errors(struct pci_dev *rcec)
>>>  
>>>  	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
>>>  	    pcie_aer_is_native(rcec))
>>> -		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
>>> +		pcie_walk_rcec_all(rcec, handles_cxl_error_iter, &handles_cxl);
>>>  
>>>  	return handles_cxl;
>>>  }
>>>  
>>> -static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>> +static void cxl_enable_rcec(struct pci_dev *rcec)
>>>  {
>>> +	/*
>>> +	 * Enable RCEC's internal error report for two cases:
>>> +	 * 1. RCiEP detected CXL.cachemem protocol errors
>>> +	 * 2. CXL root port detected CXL.cachemem protocol errors.
>>> +	 */
>>>  	if (!handles_cxl_errors(rcec))
>>>  		return;
>>>  
>>> @@ -1069,7 +1079,7 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>>  }
>>>  
>>>  #else
>>> -static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
>>> +static inline void cxl_enable_rcec(struct pci_dev *dev) { }
>>>  static inline void cxl_rch_handle_error(struct pci_dev *dev,
>>>  					struct aer_err_info *info) { }
>>>  #endif
>>> @@ -1494,7 +1504,7 @@ static int aer_probe(struct pcie_device *dev)
>>>  		return status;
>>>  	}
>>>  
>>> -	cxl_rch_enable_rcec(port);
>>> +	cxl_enable_rcec(port);
>>>  	aer_enable_rootport(rpc);
>>>  	pci_info(port, "enabled with IRQ %d\n", dev->irq);
>>>  	return 0;
> 

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Li, Ming]

On 4/16/2024 10:46 PM, Terry Bowman wrote:
> Hi Ming,
> 
> On 4/16/24 02:27, Li, Ming wrote:
>> On 3/26/2024 3:42 AM, Terry Bowman wrote:
>>> Hi Li, 
>>>
>>> I added comments below.
>>>
>>> On 3/13/24 03:35, Li Ming wrote:
>>>> Per CXl r3.1 section 12.2.2, CXL.cachemem protocol erros detected by CXL
>>>> root port could be logged in RCEC AER Extended Capability as
>>>> PCI_ERR_UNC_INTN or PCI_ERR_COR_INTERNAL. Unmask these errors for that
>>>> case.
>>>>
>>>> Signed-off-by: Li Ming <ming4.li@intel.com>
>>>> ---
>>>>  drivers/pci/pcie/aer.c | 24 +++++++++++++++++-------
>>>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>> index 42a3bd35a3e1..364c74e47273 100644
>>>> --- a/drivers/pci/pcie/aer.c
>>>> +++ b/drivers/pci/pcie/aer.c
>>>> @@ -985,7 +985,7 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>>>>  {
>>>>  	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>>>>  
>>>> -	return (pcie_ports_native || host->native_aer);
>>>> +	return (pcie_ports_native || host->native_aer) && host->is_cxl;
>>>>  }
>>>>  
>>>>  static bool is_internal_error(struct aer_err_info *info)
>>>> @@ -1041,8 +1041,13 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>>>  {
>>>>  	bool *handles_cxl = data;
>>>>  
>>>> -	if (!*handles_cxl)
>>>> -		*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
>>>> +	if (!*handles_cxl && cxl_error_is_native(dev)) {
>>>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END &&
>>>> +		    dev->rcec && is_cxl_mem_dev(dev))
>>>> +			*handles_cxl = true;
>>>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>>>> +			*handles_cxl = true;
>>>> +	}
>>> I understand a root port can be found under an RCEC. It's possible. But, does the downstream 
>>> root port forward AER to the upstream RCEC? My understanding is AER is handled and processed
>>> at the first root port/RCEC upstream from the device/RCH/USP/DSP.
>>>  
>>> Regards,
>>> Terry
>>>
>>
>> CXL r3.1 section 12.2.2 mentions this:
>>
>> "If the CXL.cachemem protocol errors detected by a CXL root port are logged as
>> CIEs or UIEs in an RCEC’s AER Extended Capability, it is recommended that the System
>> Firmware populate an RDPAS record (see Section 9.18.1.5) to establish the association
>> between the RCEC and the root port."
>>
>> I think it means that CXL root port is possible to forward its AER to RCEC.
>>
>> Thanks
>> Ming
>>
> 
> Thanks for pointing to spec details. 
> 
> In testing here, we used root port as agent to consume root port CXL protocol errors.
> The logic to handle the root port errors requires little to no AER driver changes.
> This results in a root port consuming VH protocol errors and RCEC consuming RCD 
> protocol errors. The RCEC and root port both use the PCIe port bus driver's AER service
> driver in separate instances for RCEC-RCD and root-port-VH.
> 
> The driver support is much simpler if RCEC does not handle VH protocol errors. Is there 
> a reason to forward root port VH mode protocol errors to an RCEC rather than consume 
> in the root port's AER driver and forward to CXL error handler? 
> 
> Regards,
> Terry

I agree that is simpler if only root port handle VH protocol errors, but I think that software has no chance to choose if VH protocol errors reported to RCEC or root port, it depends on platform implementation. So I think we should support both cases.


Thanks
Ming

> 
>>>>  
>>>>  	/* Non-zero terminates iteration */
>>>>  	return *handles_cxl;
>>>> @@ -1054,13 +1059,18 @@ static bool handles_cxl_errors(struct pci_dev *rcec)
>>>>  
>>>>  	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
>>>>  	    pcie_aer_is_native(rcec))
>>>> -		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
>>>> +		pcie_walk_rcec_all(rcec, handles_cxl_error_iter, &handles_cxl);
>>>>  
>>>>  	return handles_cxl;
>>>>  }
>>>>  
>>>> -static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>>> +static void cxl_enable_rcec(struct pci_dev *rcec)
>>>>  {
>>>> +	/*
>>>> +	 * Enable RCEC's internal error report for two cases:
>>>> +	 * 1. RCiEP detected CXL.cachemem protocol errors
>>>> +	 * 2. CXL root port detected CXL.cachemem protocol errors.
>>>> +	 */
>>>>  	if (!handles_cxl_errors(rcec))
>>>>  		return;
>>>>  
>>>> @@ -1069,7 +1079,7 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>>>  }
>>>>  
>>>>  #else
>>>> -static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
>>>> +static inline void cxl_enable_rcec(struct pci_dev *dev) { }
>>>>  static inline void cxl_rch_handle_error(struct pci_dev *dev,
>>>>  					struct aer_err_info *info) { }
>>>>  #endif
>>>> @@ -1494,7 +1504,7 @@ static int aer_probe(struct pcie_device *dev)
>>>>  		return status;
>>>>  	}
>>>>  
>>>> -	cxl_rch_enable_rcec(port);
>>>> +	cxl_enable_rcec(port);
>>>>  	aer_enable_rootport(rpc);
>>>>  	pci_info(port, "enabled with IRQ %d\n", dev->irq);
>>>>  	return 0;
>>

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Dan Williams]

Li, Ming wrote:
> On 4/16/2024 10:46 PM, Terry Bowman wrote:
> > The driver support is much simpler if RCEC does not handle VH protocol errors. Is there 
> > a reason to forward root port VH mode protocol errors to an RCEC rather than consume 
> > in the root port's AER driver and forward to CXL error handler? 
> > 
> I agree that is simpler if only root port handle VH protocol errors,
> but I think that software has no chance to choose if VH protocol
> errors reported to RCEC or root port, it depends on platform
> implementation. So I think we should support both cases.

The question is whether the CXL spec RDPAS behavior causes any problems
for platforms that follow PCIe rather than CXL reporting flows for
root-port errors. I.e. does it cause problems if Linux starts scanning
root ports on RCEC notifications?

I do think the lookup needs to change to be based on CXL host-bridge
detection and not CXL-type-3 endpoint detection, but otherwise it looks
like CXL spec wants to invalidate PCIe spec expectations.

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Li, Ming]

On 4/18/2024 10:57 PM, Dan Williams wrote:
> Li, Ming wrote:
>> On 4/16/2024 10:46 PM, Terry Bowman wrote:
>>> The driver support is much simpler if RCEC does not handle VH protocol errors. Is there 
>>> a reason to forward root port VH mode protocol errors to an RCEC rather than consume 
>>> in the root port's AER driver and forward to CXL error handler? 
>>>
>> I agree that is simpler if only root port handle VH protocol errors,
>> but I think that software has no chance to choose if VH protocol
>> errors reported to RCEC or root port, it depends on platform
>> implementation. So I think we should support both cases.
> 
> The question is whether the CXL spec RDPAS behavior causes any problems
> for platforms that follow PCIe rather than CXL reporting flows for
> root-port errors. I.e. does it cause problems if Linux starts scanning
> root ports on RCEC notifications?
> 
> I do think the lookup needs to change to be based on CXL host-bridge
> detection and not CXL-type-3 endpoint detection, but otherwise it looks
> like CXL spec wants to invalidate PCIe spec expectations.

Hi Dan, if my understanding is correct, the CXL host-bridge detection you mentioned is that iterating all root ports under RCEC associated bus range for RCEC reported VH protocol errors case, and the CXL-type-3 detection is that iterating all CXL-type-3 endpoint under RCEC associated bus range. is it right?

Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for CXL root port

[Dan Williams]

Li, Ming wrote:
> On 4/18/2024 10:57 PM, Dan Williams wrote:
> > Li, Ming wrote:
> >> On 4/16/2024 10:46 PM, Terry Bowman wrote:
> >>> The driver support is much simpler if RCEC does not handle VH protocol errors. Is there 
> >>> a reason to forward root port VH mode protocol errors to an RCEC rather than consume 
> >>> in the root port's AER driver and forward to CXL error handler? 
> >>>
> >> I agree that is simpler if only root port handle VH protocol errors,
> >> but I think that software has no chance to choose if VH protocol
> >> errors reported to RCEC or root port, it depends on platform
> >> implementation. So I think we should support both cases.
> > 
> > The question is whether the CXL spec RDPAS behavior causes any problems
> > for platforms that follow PCIe rather than CXL reporting flows for
> > root-port errors. I.e. does it cause problems if Linux starts scanning
> > root ports on RCEC notifications?
> > 
> > I do think the lookup needs to change to be based on CXL host-bridge
> > detection and not CXL-type-3 endpoint detection, but otherwise it looks
> > like CXL spec wants to invalidate PCIe spec expectations.
> 
> Hi Dan, if my understanding is correct, the CXL host-bridge detection
> you mentioned is that iterating all root ports under RCEC associated
> bus range for RCEC reported VH protocol errors case, and the
> CXL-type-3 detection is that iterating all CXL-type-3 endpoint under
> RCEC associated bus range. is it right?

I think this error checking needs to be tightly scoped to only scan for
CXL.cachemem errors and not CXL.io or typical PCIe errors. That way we
are not technically running afoul of the PCIe expectations that *PCIe*
root-port errors are only reported by their local AER block and not an
RCEC.

So the scanning should be limited to just the root-ports that have
negotiated a CXL.cachemem link.

[RFC PATCH 4/6] PCI/AER: Extend RCH RAS error handling to support VH topology case

[Li Ming]

When RCEC captures CXL.cachemem protocol errors detected by CXL root
port, the recommendation from CXL r3.1 9.18.1.5 is :

	"Probe all CXL Downstream Ports and determine whether they have logged an
	error in the CXL.io or CXL.cachemem status registers."

The flow is similar with RCH RAS error handling, so reuse it to support
above case.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/pci/pcie/aer.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 364c74e47273..79bfa5fb78f4 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -996,11 +996,15 @@ static bool is_internal_error(struct aer_err_info *info)
 	return info->status & PCI_ERR_UNC_INTN;
 }
 
-static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
+static int cxl_handle_error_iter(struct pci_dev *dev, void *data)
 {
 	struct aer_err_info *info = (struct aer_err_info *)data;
 	const struct pci_error_handlers *err_handler;
 
+	/* Skip the RCiEP devices not associating with RCEC */
+	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
+	    !dev->rcec)
+		return 0;
 	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
 		return 0;
 
@@ -1025,16 +1029,16 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
 	return 0;
 }
 
-static void cxl_rch_handle_error(struct pci_dev *dev, struct aer_err_info *info)
+static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info)
 {
 	/*
 	 * Internal errors of an RCEC indicate an AER error in an
-	 * RCH's downstream port. Check and handle them in the CXL.mem
-	 * device driver.
+	 * RCH's downstream port or a CXL root port. Check and handle
+	 * them in the CXL.mem device driver.
 	 */
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC &&
 	    is_internal_error(info))
-		pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info);
+		pcie_walk_rcec_all(dev, cxl_handle_error_iter, info);
 }
 
 static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
@@ -1080,8 +1084,8 @@ static void cxl_enable_rcec(struct pci_dev *rcec)
 
 #else
 static inline void cxl_enable_rcec(struct pci_dev *dev) { }
-static inline void cxl_rch_handle_error(struct pci_dev *dev,
-					struct aer_err_info *info) { }
+static inline void cxl_handle_error(struct pci_dev *dev,
+				    struct aer_err_info *info) { }
 #endif
 
 /**
@@ -1119,7 +1123,7 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
 
 static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
 {
-	cxl_rch_handle_error(dev, info);
+	cxl_handle_error(dev, info);
 	pci_aer_handle_error(dev, info);
 	pci_dev_put(dev);
 }
-- 
2.40.1

Re: [RFC PATCH 4/6] PCI/AER: Extend RCH RAS error handling to support VH topology case

[Dan Williams]

Li Ming wrote:
> When RCEC captures CXL.cachemem protocol errors detected by CXL root
> port, the recommendation from CXL r3.1 9.18.1.5 is :
> 
> 	"Probe all CXL Downstream Ports and determine whether they have logged an
> 	error in the CXL.io or CXL.cachemem status registers."
> 
> The flow is similar with RCH RAS error handling, so reuse it to support
> above case.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/pci/pcie/aer.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 364c74e47273..79bfa5fb78f4 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -996,11 +996,15 @@ static bool is_internal_error(struct aer_err_info *info)
>  	return info->status & PCI_ERR_UNC_INTN;
>  }
>  
> -static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
> +static int cxl_handle_error_iter(struct pci_dev *dev, void *data)
>  {
>  	struct aer_err_info *info = (struct aer_err_info *)data;
>  	const struct pci_error_handlers *err_handler;
>  
> +	/* Skip the RCiEP devices not associating with RCEC */
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
> +	    !dev->rcec)
> +		return 0;
>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>  		return 0;

is_cxl_mem_dev(dev) will always be false in the VH case, so how does
this change help the VH case?

Re: [RFC PATCH 4/6] PCI/AER: Extend RCH RAS error handling to support VH topology case

[Li, Ming]

On 3/15/2024 10:30 AM, Dan Williams wrote:
> Li Ming wrote:
>> When RCEC captures CXL.cachemem protocol errors detected by CXL root
>> port, the recommendation from CXL r3.1 9.18.1.5 is :
>>
>> 	"Probe all CXL Downstream Ports and determine whether they have logged an
>> 	error in the CXL.io or CXL.cachemem status registers."
>>
>> The flow is similar with RCH RAS error handling, so reuse it to support
>> above case.
>>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>>  drivers/pci/pcie/aer.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 364c74e47273..79bfa5fb78f4 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -996,11 +996,15 @@ static bool is_internal_error(struct aer_err_info *info)
>>  	return info->status & PCI_ERR_UNC_INTN;
>>  }
>>  
>> -static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>> +static int cxl_handle_error_iter(struct pci_dev *dev, void *data)
>>  {
>>  	struct aer_err_info *info = (struct aer_err_info *)data;
>>  	const struct pci_error_handlers *err_handler;
>>  
>> +	/* Skip the RCiEP devices not associating with RCEC */
>> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
>> +	    !dev->rcec)
>> +		return 0;
>>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>>  		return 0;
> 
> is_cxl_mem_dev(dev) will always be false in the VH case, so how does
> this change help the VH case?

Hi Dan,

I think it won't be false if the CXL memory device is an endpoint.
pcie_walk_rcec_all() will walk all pci_dev in RCEC assocaited bus ranges. So these two checkings can help us to filter:
1. CXL memory device is an RCiEP associated with RCEC in the RCH case
2. CXL memory device is not an RCiEP, so it should be an endpoint in the VH case.

Re: [RFC PATCH 4/6] PCI/AER: Extend RCH RAS error handling to support VH topology case

[Dan Williams]

Li, Ming wrote:
> On 3/15/2024 10:30 AM, Dan Williams wrote:
> > Li Ming wrote:
> >> When RCEC captures CXL.cachemem protocol errors detected by CXL root
> >> port, the recommendation from CXL r3.1 9.18.1.5 is :
> >>
> >> 	"Probe all CXL Downstream Ports and determine whether they have logged an
> >> 	error in the CXL.io or CXL.cachemem status registers."
> >>
> >> The flow is similar with RCH RAS error handling, so reuse it to support
> >> above case.
> >>
> >> Signed-off-by: Li Ming <ming4.li@intel.com>
> >> ---
> >>  drivers/pci/pcie/aer.c | 20 ++++++++++++--------
> >>  1 file changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >> index 364c74e47273..79bfa5fb78f4 100644
> >> --- a/drivers/pci/pcie/aer.c
> >> +++ b/drivers/pci/pcie/aer.c
> >> @@ -996,11 +996,15 @@ static bool is_internal_error(struct aer_err_info *info)
> >>  	return info->status & PCI_ERR_UNC_INTN;
> >>  }
> >>  
> >> -static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
> >> +static int cxl_handle_error_iter(struct pci_dev *dev, void *data)
> >>  {
> >>  	struct aer_err_info *info = (struct aer_err_info *)data;
> >>  	const struct pci_error_handlers *err_handler;
> >>  
> >> +	/* Skip the RCiEP devices not associating with RCEC */
> >> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
> >> +	    !dev->rcec)
> >> +		return 0;
> >>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
> >>  		return 0;
> > 
> > is_cxl_mem_dev(dev) will always be false in the VH case, so how does
> > this change help the VH case?
> 
> Hi Dan,
> 
> I think it won't be false if the CXL memory device is an endpoint.
> pcie_walk_rcec_all() will walk all pci_dev in RCEC assocaited bus ranges. So these two checkings can help us to filter:
> 1. CXL memory device is an RCiEP associated with RCEC in the RCH case
> 2. CXL memory device is not an RCiEP, so it should be an endpoint in the VH case.

It will be an endpoint, but I though cxl_handle_error_iter() is only
called for RCIEPs and RPs that are share a bus range with the RCEC. The
endpoint in the VH case is downstream of the RP.

I had been assuming that pci_walk_bus() limits itself to buses within
the Root Complex however it descends the entire bus hierarchy so this
implementation will walk the entire topology on all root ports
associated with the RCEC looking for any CXL device. That feels wrong.

I would expect that this limits it self to only finding root ports and
then only proceeding if that root port has a directly attached CXL
device.

Note, when you send a v2 of this RFC be sure to copy linux-pci for these
core changes to PCI error handling.

Re: [RFC PATCH 4/6] PCI/AER: Extend RCH RAS error handling to support VH topology case

[Li, Ming]

On 3/15/2024 12:05 PM, Dan Williams wrote:
> Li, Ming wrote:
>> On 3/15/2024 10:30 AM, Dan Williams wrote:
>>> Li Ming wrote:
>>>> When RCEC captures CXL.cachemem protocol errors detected by CXL root
>>>> port, the recommendation from CXL r3.1 9.18.1.5 is :
>>>>
>>>> 	"Probe all CXL Downstream Ports and determine whether they have logged an
>>>> 	error in the CXL.io or CXL.cachemem status registers."
>>>>
>>>> The flow is similar with RCH RAS error handling, so reuse it to support
>>>> above case.
>>>>
>>>> Signed-off-by: Li Ming <ming4.li@intel.com>
>>>> ---
>>>>  drivers/pci/pcie/aer.c | 20 ++++++++++++--------
>>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>> index 364c74e47273..79bfa5fb78f4 100644
>>>> --- a/drivers/pci/pcie/aer.c
>>>> +++ b/drivers/pci/pcie/aer.c
>>>> @@ -996,11 +996,15 @@ static bool is_internal_error(struct aer_err_info *info)
>>>>  	return info->status & PCI_ERR_UNC_INTN;
>>>>  }
>>>>  
>>>> -static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>>>> +static int cxl_handle_error_iter(struct pci_dev *dev, void *data)
>>>>  {
>>>>  	struct aer_err_info *info = (struct aer_err_info *)data;
>>>>  	const struct pci_error_handlers *err_handler;
>>>>  
>>>> +	/* Skip the RCiEP devices not associating with RCEC */
>>>> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
>>>> +	    !dev->rcec)
>>>> +		return 0;
>>>>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>>>>  		return 0;
>>>
>>> is_cxl_mem_dev(dev) will always be false in the VH case, so how does
>>> this change help the VH case?
>>
>> Hi Dan,
>>
>> I think it won't be false if the CXL memory device is an endpoint.
>> pcie_walk_rcec_all() will walk all pci_dev in RCEC assocaited bus ranges. So these two checkings can help us to filter:
>> 1. CXL memory device is an RCiEP associated with RCEC in the RCH case
>> 2. CXL memory device is not an RCiEP, so it should be an endpoint in the VH case.
> 
> It will be an endpoint, but I though cxl_handle_error_iter() is only
> called for RCIEPs and RPs that are share a bus range with the RCEC. The
> endpoint in the VH case is downstream of the RP.
> 
> I had been assuming that pci_walk_bus() limits itself to buses within
> the Root Complex however it descends the entire bus hierarchy so this
> implementation will walk the entire topology on all root ports
> associated with the RCEC looking for any CXL device. That feels wrong.
> 
> I would expect that this limits it self to only finding root ports and
> then only proceeding if that root port has a directly attached CXL
> device.
> 
Got it, will change it in v2, thank you.

> Note, when you send a v2 of this RFC be sure to copy linux-pci for these
> core changes to PCI error handling.
Sure, I made a mistake here.

Re: [RFC PATCH 4/6] PCI/AER: Extend RCH RAS error handling to support VH topology case

[Terry Bowman]

Hi Li,

I added comments below

On 3/13/24 03:36, Li Ming wrote:
> When RCEC captures CXL.cachemem protocol errors detected by CXL root
> port, the recommendation from CXL r3.1 9.18.1.5 is :
> 
> 	"Probe all CXL Downstream Ports and determine whether they have logged an
> 	error in the CXL.io or CXL.cachemem status registers."
> 
> The flow is similar with RCH RAS error handling, so reuse it to support
> above case.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/pci/pcie/aer.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 364c74e47273..79bfa5fb78f4 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -996,11 +996,15 @@ static bool is_internal_error(struct aer_err_info *info)
>  	return info->status & PCI_ERR_UNC_INTN;
>  }
>  
> -static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
> +static int cxl_handle_error_iter(struct pci_dev *dev, void *data)
>  {
>  	struct aer_err_info *info = (struct aer_err_info *)data;
>  	const struct pci_error_handlers *err_handler;
>  
> +	/* Skip the RCiEP devices not associating with RCEC */
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
> +	    !dev->rcec)
> +		return 0>  	if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>  		return 0;
>  
> @@ -1025,16 +1029,16 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>  	return 0;
>  }
>  
> -static void cxl_rch_handle_error(struct pci_dev *dev, struct aer_err_info *info)
> +static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>  {
>  	/*
>  	 * Internal errors of an RCEC indicate an AER error in an
> -	 * RCH's downstream port. Check and handle them in the CXL.mem
> -	 * device driver.
> +	 * RCH's downstream port or a CXL root port. Check and handle
> +	 * them in the CXL.mem device driver.
>  	 */

"Internal errors of an RCEC indicate an AER error in an RCH's downstream port or a CXL root port."

Might be more correct to restate as:

"AER internal errors are used by root ports and RCECs to indicate AER in downstream CXL ports (RCH, USP, DSP) or devices"

Regards,
Terry

[RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()

[Li Ming]

Introduce a new helper function called put_cxl_port() to instead of the
put_device() in order to release the device reference of struct cxl_port
got via get_device() in cxl_pci/mem_find_port().

Besides, use scope-based resource management __free() to drop the open
coded put_device() for each cxl_pci/mem_find_port().

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/cxl/core/pci.c  |  6 ++----
 drivers/cxl/core/port.c |  9 +++++++++
 drivers/cxl/cxl.h       |  2 ++
 drivers/cxl/mem.c       |  5 ++---
 drivers/cxl/pci.c       | 12 +++++-------
 5 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 6c9c8d92f8f7..7254484330d2 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	struct aer_capability_regs aer_regs;
 	struct cxl_dport *dport;
-	struct cxl_port *port;
 	int severity;
+	struct cxl_port *port __free(put_cxl_port) =
+		cxl_pci_find_port(pdev, &dport);
 
-	port = cxl_pci_find_port(pdev, &dport);
 	if (!port)
 		return;
 
-	put_device(&port->dev);
-
 	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
 		return;
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e59d9d37aa65..6e2fc2fce7c9 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
 
+void put_cxl_port(struct cxl_port *port)
+{
+	if (!port)
+		return;
+
+	put_device(&port->dev);
+}
+EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL);
+
 static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 				    struct cxl_port *port, int *target_map)
 {
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6017c0c57b4..476158782e3e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
 				   struct cxl_dport **dport);
 struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
 				   struct cxl_dport **dport);
+void put_cxl_port(struct cxl_port *port);
+DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T))
 bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
 
 struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c5c9d8e0d88d..5aaa8ee2a46d 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct device *endpoint_parent;
-	struct cxl_port *parent_port;
 	struct cxl_dport *dport;
 	struct dentry *dentry;
 	int rc;
@@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	parent_port = cxl_mem_find_port(cxlmd, &dport);
+	struct cxl_port *parent_port __free(put_cxl_port) =
+		cxl_mem_find_port(cxlmd, &dport);
 	if (!parent_port) {
 		dev_err(dev, "CXL port topology not found\n");
 		return -ENXIO;
@@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev)
 	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
 unlock:
 	device_unlock(endpoint_parent);
-	put_device(&parent_port->dev);
 	if (rc)
 		return rc;
 
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4fd1f207c84e..d0ec8c5b1e99 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
 static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
 				  struct cxl_register_map *map)
 {
-	struct cxl_port *port;
 	struct cxl_dport *dport;
 	resource_size_t component_reg_phys;
+	struct cxl_port *port __free(put_cxl_port) =
+		cxl_pci_find_port(pdev, &dport);
+
+	if (!port)
+		return -EPROBE_DEFER;
 
 	*map = (struct cxl_register_map) {
 		.host = &pdev->dev,
 		.resource = CXL_RESOURCE_NONE,
 	};
 
-	port = cxl_pci_find_port(pdev, &dport);
-	if (!port)
-		return -EPROBE_DEFER;
-
 	component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
 
-	put_device(&port->dev);
-
 	if (component_reg_phys == CXL_RESOURCE_NONE)
 		return -ENXIO;
 
-- 
2.40.1

Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()

[Dan Williams]

Li Ming wrote:
> Introduce a new helper function called put_cxl_port() to instead of the
> put_device() in order to release the device reference of struct cxl_port
> got via get_device() in cxl_pci/mem_find_port().
> 
> Besides, use scope-based resource management __free() to drop the open
> coded put_device() for each cxl_pci/mem_find_port().
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/cxl/core/pci.c  |  6 ++----
>  drivers/cxl/core/port.c |  9 +++++++++
>  drivers/cxl/cxl.h       |  2 ++
>  drivers/cxl/mem.c       |  5 ++---
>  drivers/cxl/pci.c       | 12 +++++-------
>  5 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 6c9c8d92f8f7..7254484330d2 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	struct aer_capability_regs aer_regs;
>  	struct cxl_dport *dport;
> -	struct cxl_port *port;
>  	int severity;
> +	struct cxl_port *port __free(put_cxl_port) =
> +		cxl_pci_find_port(pdev, &dport);
>  
> -	port = cxl_pci_find_port(pdev, &dport);
>  	if (!port)

Keep the declaration separated from the rest of the variable
declarations like this:

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0df09bd79408..2d1ef35fe99c 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -916,12 +916,11 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
        struct cxl_port *port;
        int severity;
 
-       port = cxl_pci_find_port(pdev, &dport);
+       struct cxl_port *port __free(put_cxl_port) =
+               cxl_pci_find_port(pdev, &dport);
        if (!port)
                return;
 
-       put_device(&port->dev);
-
        if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
                return;
 
>  		return;
>  
> -	put_device(&port->dev);
> -
>  	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
>  		return;
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e59d9d37aa65..6e2fc2fce7c9 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
>  
> +void put_cxl_port(struct cxl_port *port)
> +{
> +	if (!port)
> +		return;
> +
> +	put_device(&port->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL);
> +
>  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>  				    struct cxl_port *port, int *target_map)
>  {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b6017c0c57b4..476158782e3e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
>  				   struct cxl_dport **dport);
>  struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>  				   struct cxl_dport **dport);
> +void put_cxl_port(struct cxl_port *port);
> +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T))
>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>  
>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..5aaa8ee2a46d 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct device *endpoint_parent;
> -	struct cxl_port *parent_port;
>  	struct cxl_dport *dport;
>  	struct dentry *dentry;
>  	int rc;
> @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	parent_port = cxl_mem_find_port(cxlmd, &dport);
> +	struct cxl_port *parent_port __free(put_cxl_port) =
> +		cxl_mem_find_port(cxlmd, &dport);
>  	if (!parent_port) {
>  		dev_err(dev, "CXL port topology not found\n");
>  		return -ENXIO;
> @@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev)
>  	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>  unlock:
>  	device_unlock(endpoint_parent);
> -	put_device(&parent_port->dev);
>  	if (rc)
>  		return rc;

When converting a function to use cleanup helpers, it should try to
remove all gotos, something like:

@@ -159,21 +159,18 @@ static int cxl_mem_probe(struct device *dev)
 
        cxl_setup_parent_dport(dev, dport);
 
-       device_lock(endpoint_parent);
-       if (!endpoint_parent->driver) {
-               dev_err(dev, "CXL port topology %s not enabled\n",
-                       dev_name(endpoint_parent));
-               rc = -ENXIO;
-               goto unlock;
+       scoped_guard(device, endpoint_parent) {
+               if (!endpoint_parent->driver) {
+                       dev_err(dev, "CXL port topology %s not enabled\n",
+                               dev_name(endpoint_parent));
+                       return -ENXIO;
+               }
+
+               rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
+               if (rc)
+                       return rc;
        }
 
-       rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
-unlock:
-       device_unlock(endpoint_parent);
-       put_device(&parent_port->dev);
-       if (rc)
-               return rc;
-
        if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
                rc = devm_cxl_add_nvdimm(cxlmd);
                if (rc == -ENODEV)


>  
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4fd1f207c84e..d0ec8c5b1e99 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
>  static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>  				  struct cxl_register_map *map)
>  {
> -	struct cxl_port *port;
>  	struct cxl_dport *dport;
>  	resource_size_t component_reg_phys;
> +	struct cxl_port *port __free(put_cxl_port) =
> +		cxl_pci_find_port(pdev, &dport);
> +
> +	if (!port)
> +		return -EPROBE_DEFER;
>  
>  	*map = (struct cxl_register_map) {
>  		.host = &pdev->dev,
>  		.resource = CXL_RESOURCE_NONE,
>  	};
>  
> -	port = cxl_pci_find_port(pdev, &dport);
> -	if (!port)
> -		return -EPROBE_DEFER;
> -

Here again don't move the cxl_pci_find_port() earlier in the function,
keep it after the assignment of @map.

Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()

[Li, Ming]

On 3/15/2024 10:24 AM, Dan Williams wrote:
> Li Ming wrote:
>> Introduce a new helper function called put_cxl_port() to instead of the
>> put_device() in order to release the device reference of struct cxl_port
>> got via get_device() in cxl_pci/mem_find_port().
>>
>> Besides, use scope-based resource management __free() to drop the open
>> coded put_device() for each cxl_pci/mem_find_port().
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>>  drivers/cxl/core/pci.c  |  6 ++----
>>  drivers/cxl/core/port.c |  9 +++++++++
>>  drivers/cxl/cxl.h       |  2 ++
>>  drivers/cxl/mem.c       |  5 ++---
>>  drivers/cxl/pci.c       | 12 +++++-------
>>  5 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 6c9c8d92f8f7..7254484330d2 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
>>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>>  	struct aer_capability_regs aer_regs;
>>  	struct cxl_dport *dport;
>> -	struct cxl_port *port;
>>  	int severity;
>> +	struct cxl_port *port __free(put_cxl_port) =
>> +		cxl_pci_find_port(pdev, &dport);
>>  
>> -	port = cxl_pci_find_port(pdev, &dport);
>>  	if (!port)
> 
> Keep the declaration separated from the rest of the variable
> declarations like this:
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0df09bd79408..2d1ef35fe99c 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -916,12 +916,11 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
>         struct cxl_port *port;
>         int severity;
>  
> -       port = cxl_pci_find_port(pdev, &dport);
> +       struct cxl_port *port __free(put_cxl_port) =
> +               cxl_pci_find_port(pdev, &dport);
>         if (!port)
>                 return;
>  
> -       put_device(&port->dev);
> -
>         if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
>                 return;
>  
>>  		return;
>>  
>> -	put_device(&port->dev);
>> -
>>  	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
>>  		return;
>>  
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e59d9d37aa65..6e2fc2fce7c9 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
>>  
>> +void put_cxl_port(struct cxl_port *port)
>> +{
>> +	if (!port)
>> +		return;
>> +
>> +	put_device(&port->dev);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL);
>> +
>>  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>>  				    struct cxl_port *port, int *target_map)
>>  {
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index b6017c0c57b4..476158782e3e 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
>>  				   struct cxl_dport **dport);
>>  struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>>  				   struct cxl_dport **dport);
>> +void put_cxl_port(struct cxl_port *port);
>> +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T))
>>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>>  
>>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index c5c9d8e0d88d..5aaa8ee2a46d 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
>>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  	struct device *endpoint_parent;
>> -	struct cxl_port *parent_port;
>>  	struct cxl_dport *dport;
>>  	struct dentry *dentry;
>>  	int rc;
>> @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
>>  	if (rc)
>>  		return rc;
>>  
>> -	parent_port = cxl_mem_find_port(cxlmd, &dport);
>> +	struct cxl_port *parent_port __free(put_cxl_port) =
>> +		cxl_mem_find_port(cxlmd, &dport);
>>  	if (!parent_port) {
>>  		dev_err(dev, "CXL port topology not found\n");
>>  		return -ENXIO;
>> @@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev)
>>  	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>>  unlock:
>>  	device_unlock(endpoint_parent);
>> -	put_device(&parent_port->dev);
>>  	if (rc)
>>  		return rc;
> 
> When converting a function to use cleanup helpers, it should try to
> remove all gotos, something like:
> 
> @@ -159,21 +159,18 @@ static int cxl_mem_probe(struct device *dev)
>  
>         cxl_setup_parent_dport(dev, dport);
>  
> -       device_lock(endpoint_parent);
> -       if (!endpoint_parent->driver) {
> -               dev_err(dev, "CXL port topology %s not enabled\n",
> -                       dev_name(endpoint_parent));
> -               rc = -ENXIO;
> -               goto unlock;
> +       scoped_guard(device, endpoint_parent) {
> +               if (!endpoint_parent->driver) {
> +                       dev_err(dev, "CXL port topology %s not enabled\n",
> +                               dev_name(endpoint_parent));
> +                       return -ENXIO;
> +               }
> +
> +               rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
> +               if (rc)
> +                       return rc;
>         }
>  
> -       rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
> -unlock:
> -       device_unlock(endpoint_parent);
> -       put_device(&parent_port->dev);
> -       if (rc)
> -               return rc;
> -
>         if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
>                 rc = devm_cxl_add_nvdimm(cxlmd);
>                 if (rc == -ENODEV)
> 
> 
>>  
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 4fd1f207c84e..d0ec8c5b1e99 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
>>  static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>>  				  struct cxl_register_map *map)
>>  {
>> -	struct cxl_port *port;
>>  	struct cxl_dport *dport;
>>  	resource_size_t component_reg_phys;
>> +	struct cxl_port *port __free(put_cxl_port) =
>> +		cxl_pci_find_port(pdev, &dport);
>> +
>> +	if (!port)
>> +		return -EPROBE_DEFER;
>>  
>>  	*map = (struct cxl_register_map) {
>>  		.host = &pdev->dev,
>>  		.resource = CXL_RESOURCE_NONE,
>>  	};
>>  
>> -	port = cxl_pci_find_port(pdev, &dport);
>> -	if (!port)
>> -		return -EPROBE_DEFER;
>> -
> 
> Here again don't move the cxl_pci_find_port() earlier in the function,
> keep it after the assignment of @map.

Thanks, I will do that.

[RFC PATCH 6/6] cxl/pci: Support to handle root port RAS errors captured by RCEC

[Li Ming]

The CXL subsystem already supports RCH RAS Error handling that has a
dependency on the RCEC. Reuse and extend that RCH topology support to
handle the errors detected by root port and logged in RCEC.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/cxl/core/pci.c | 83 ++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 27 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 7254484330d2..154812f1f450 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -837,18 +837,6 @@ void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_dport, CXL);
 
-static void cxl_handle_rdport_cor_ras(struct cxl_dev_state *cxlds,
-					  struct cxl_dport *dport)
-{
-	return __cxl_handle_cor_ras(cxlds, dport->regs.ras);
-}
-
-static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
-				       struct cxl_dport *dport)
-{
-	return __cxl_handle_ras(cxlds, dport->regs.ras);
-}
-
 /*
  * Copy the AER capability registers using 32 bit read accesses.
  * This is necessary because RCRB AER capability is MMIO mapped. Clear the
@@ -897,10 +885,45 @@ static bool cxl_rch_get_aer_severity(struct aer_capability_regs *aer_regs,
 	return false;
 }
 
-static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
+/* Get AER severity from CXL RAS Capability */
+static bool cxl_ras_get_aer_severity(void __iomem *ras_base, int *severity)
+{
+	void __iomem *addr;
+	u32 ue_severity;
+	u32 status;
+
+	if (!ras_base)
+		return false;
+
+	addr = ras_base + CXL_RAS_UNCORRECTABLE_STATUS_OFFSET;
+	status = readl(addr);
+	addr = ras_base + CXL_RAS_UNCORRECTABLE_SEVERITY_OFFSET;
+	ue_severity = readl(addr);
+	status &= CXL_RAS_UNCORRECTABLE_STATUS_MASK;
+	if (status) {
+		if (status & ue_severity)
+			*severity = AER_FATAL;
+		else
+			*severity = AER_NONFATAL;
+
+		return true;
+	}
+
+	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
+	status = readl(addr);
+	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
+		*severity = AER_CORRECTABLE;
+		return true;
+	}
+
+	return false;
+}
+
+static void cxl_handle_dport_errors(struct cxl_dev_state *cxlds)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	struct aer_capability_regs aer_regs;
+	struct pci_dev *dport_pdev;
 	struct cxl_dport *dport;
 	int severity;
 	struct cxl_port *port __free(put_cxl_port) =
@@ -909,31 +932,38 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
 	if (!port)
 		return;
 
-	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
-		return;
-
-	if (!cxl_rch_get_aer_severity(&aer_regs, &severity))
-		return;
+	if (cxlds->rcd) {
+		if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
+			return;
 
-	pci_print_aer(pdev, severity, &aer_regs);
+		if (!cxl_rch_get_aer_severity(&aer_regs, &severity))
+			return;
+		pci_print_aer(pdev, severity, &aer_regs);
+	} else {
+		dport_pdev = to_pci_dev(dport->dport_dev);
+		/* TODO: add support for switch downstream port error handling */
+		if (pci_pcie_type(dport_pdev) != PCI_EXP_TYPE_ROOT_PORT)
+			return;
+		if (!cxl_ras_get_aer_severity(dport->regs.ras, &severity))
+			return;
+	}
 
 	if (severity == AER_CORRECTABLE)
-		cxl_handle_rdport_cor_ras(cxlds, dport);
+		__cxl_handle_cor_ras(cxlds, dport->regs.ras);
 	else
-		cxl_handle_rdport_ras(cxlds, dport);
+		__cxl_handle_ras(cxlds, dport->regs.ras);
+
 }
 
 #else
-static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) { }
+static void cxl_handle_dport_errors(struct cxl_dev_state *cxlds) { }
 #endif
 
 void cxl_cor_error_detected(struct pci_dev *pdev)
 {
 	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
 
-	if (cxlds->rcd)
-		cxl_handle_rdport_errors(cxlds);
-
+	cxl_handle_dport_errors(cxlds);
 	cxl_handle_endpoint_cor_ras(cxlds);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, CXL);
@@ -946,8 +976,7 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
 	struct device *dev = &cxlmd->dev;
 	bool ue;
 
-	if (cxlds->rcd)
-		cxl_handle_rdport_errors(cxlds);
+	cxl_handle_dport_errors(cxlds);
 
 	/*
 	 * A frozen channel indicates an impending reset which is fatal to
-- 
2.40.1

Re: [RFC PATCH 0/6] Add support for root port RAS error handling

[Dan Williams]

Li Ming wrote:
> Protocol errors signaled to a CXL root port may be captured by a Root
> Complex Event Collector(RCEC). If those errors are not cleared and
> reported the system owner loses forensic information for system failure
> analysis.
> 
> Per CXL r3.1 section 9.18.1.5, the recommendation for this case from CXL
> specification is the 'Else' statement in 'IMPLEMENTATION NODE' under
> 'Table 9-24 RDPAS Structure':
> 
> 	"Probe all CXL Downstream Ports and determine whether they have logged an
> 	error in the CXL.io or CXL.cachemem status registers."
> 
> The CXL subsystem already supports RCH RAS Error handling that has a
> dependency on the RCEC. Reuse and extend that RCH topoogy support to
> handle reported errors in the VH topology case. The implementation is
> composed of:
> * Provide a new interface from RCEC side to support walk all devices
>   under RCEC and RCEC associated bus range. PCIe AER core uses this
>   interface to walk all CXL endpoints and all CXL root ports under the
>   bus ranges.
> * Update the PCIe AER core to enable Uncorrectable Internal Errors and
>   Correctable Internal Errors report for root ports.

Thanks for the above background.

> * Invoke the cxl_pci error handler for RCEC reported errors.

So what do you expect happens when a switch is involved? In the RCH case
it knows that the only thing that can fire RCEC is a root complex
integrated endpoint implementation driven by cxl_pci. In the VH case it
could be a switch.

> * Handle root-port errors in the cxl_pci handler when the device is
>   direct attached.

I do expect direct-attach to be a predominant use case, but I want to
make sure that the implementation at least does not make the switch port
error handling case more difficult to implement.

Re: [RFC PATCH 0/6] Add support for root port RAS error handling

[Li, Ming]

On 3/15/2024 9:45 AM, Dan Williams wrote:
> Li Ming wrote:
>> Protocol errors signaled to a CXL root port may be captured by a Root
>> Complex Event Collector(RCEC). If those errors are not cleared and
>> reported the system owner loses forensic information for system failure
>> analysis.
>>
>> Per CXL r3.1 section 9.18.1.5, the recommendation for this case from CXL
>> specification is the 'Else' statement in 'IMPLEMENTATION NODE' under
>> 'Table 9-24 RDPAS Structure':
>>
>> 	"Probe all CXL Downstream Ports and determine whether they have logged an
>> 	error in the CXL.io or CXL.cachemem status registers."
>>
>> The CXL subsystem already supports RCH RAS Error handling that has a
>> dependency on the RCEC. Reuse and extend that RCH topoogy support to
>> handle reported errors in the VH topology case. The implementation is
>> composed of:
>> * Provide a new interface from RCEC side to support walk all devices
>>   under RCEC and RCEC associated bus range. PCIe AER core uses this
>>   interface to walk all CXL endpoints and all CXL root ports under the
>>   bus ranges.
>> * Update the PCIe AER core to enable Uncorrectable Internal Errors and
>>   Correctable Internal Errors report for root ports.
> 
> Thanks for the above background.
> 
>> * Invoke the cxl_pci error handler for RCEC reported errors.
> 
> So what do you expect happens when a switch is involved? In the RCH case
> it knows that the only thing that can fire RCEC is a root complex
> integrated endpoint implementation driven by cxl_pci. In the VH case it
> could be a switch.
> 
>> * Handle root-port errors in the cxl_pci handler when the device is
>>   direct attached.
> 
> I do expect direct-attach to be a predominant use case, but I want to
> make sure that the implementation at least does not make the switch port
> error handling case more difficult to implement.

Hi Dan,

Currently, A rough idea I have is that:
If a CXL switch connected to the CXL RP, there should be two cases,
1. no CXL memory device connected to the switch, in this case, I'm not sure whether CXL.cachemem protocol errors is still possibly happened between RP and switch without CXL memory device. If not, maybe we don't need to consider such case?

2. a CXL memory device connected to the switch. I think cxl_pci error handler could also help to handle CXL.cachemem protocol errors happened in switch USP/DSP.

Re: [RFC PATCH 0/6] Add support for root port RAS error handling

[Dan Williams]

Li, Ming wrote:
[..]
> > I do expect direct-attach to be a predominant use case, but I want to
> > make sure that the implementation at least does not make the switch port
> > error handling case more difficult to implement.
> 
> Hi Dan,
> 
> Currently, A rough idea I have is that:
> If a CXL switch connected to the CXL RP, there should be two cases,
> 1. no CXL memory device connected to the switch, in this case, I'm not
> sure whether CXL.cachemem protocol errors is still possibly happened
> between RP and switch without CXL memory device. If not, maybe we
> don't need to consider such case?

Protocol errors can happen between any 2 ports, just like PCI protocol
errors.

> 2. a CXL memory device connected to the switch. I think cxl_pci error
> handler could also help to handle CXL.cachemem protocol errors
> happened in switch USP/DSP.

No, for 2 reasons:

* The cxl_pci driver is only for general CXL type-3 memory
  expanders. Even though no CXL.cache devices have upstream drivers they
  do exist and they would experience protocol errors that the PCI core
  needs to consider.

* When a switch is present it is possible to have a protocol error
  between the switch upstream port and the root port, and not between
  the switch downstream port and the endpoint.

The more I think about it I do not think it is appropriate for cxl_pci
to be involved in clearing root port errors in the VH case, it only
works the RCH case because of the way the device and the root-port get
combined.

Re: [RFC PATCH 0/6] Add support for root port RAS error handling

[Li, Ming]

On 3/16/2024 2:21 AM, Dan Williams wrote:
> Li, Ming wrote:
> [..]
>>> I do expect direct-attach to be a predominant use case, but I want to
>>> make sure that the implementation at least does not make the switch port
>>> error handling case more difficult to implement.
>>
>> Hi Dan,
>>
>> Currently, A rough idea I have is that:
>> If a CXL switch connected to the CXL RP, there should be two cases,
>> 1. no CXL memory device connected to the switch, in this case, I'm not
>> sure whether CXL.cachemem protocol errors is still possibly happened
>> between RP and switch without CXL memory device. If not, maybe we
>> don't need to consider such case?
> 
> Protocol errors can happen between any 2 ports, just like PCI protocol
> errors.

Seems like if we want to handle CXL protocol error for such case on CXL driver side, we need some changes for CXL switch port/dport enumeration, I remembered that switch USP and DSP are enumerated only when a CXL memory device attached.

> 
>> 2. a CXL memory device connected to the switch. I think cxl_pci error
>> handler could also help to handle CXL.cachemem protocol errors
>> happened in switch USP/DSP.
> 
> No, for 2 reasons:
> 
> * The cxl_pci driver is only for general CXL type-3 memory
>   expanders. Even though no CXL.cache devices have upstream drivers they
>   do exist and they would experience protocol errors that the PCI core
>   needs to consider.
> 
> * When a switch is present it is possible to have a protocol error
>   between the switch upstream port and the root port, and not between
>   the switch downstream port and the endpoint.
> 
> The more I think about it I do not think it is appropriate for cxl_pci
> to be involved in clearing root port errors in the VH case, it only
> works the RCH case because of the way the device and the root-port get
> combined.

Thank you for your explanation. 

I think I need some time to consider a appropriate proposal. maybe we can register a CXL error handler into struct pci_driver during CXL port/dport enumeration for root port and switch cases, PCIe AER driver will invoke this registered CXL error handler to handle errors on CXL subsystem side. But I think this solution also meets above issue(current CXL subsystem won't enumerate CXL switch port/dport without CXL mem device).