diff mbox series

[v3,05/15] PCI/AER: Add CXL PCIe port correctable error support in AER service driver

Message ID 20241113215429.3177981-6-terry.bowman@amd.com (mailing list archive)
State New
Headers show
Series Enable CXL PCIe port protocol error handling and logging | expand

Commit Message

Bowman, Terry Nov. 13, 2024, 9:54 p.m. UTC 
The AER service driver supports handling downstream port protocol errors in
restricted CXL host (RCH) mode also known as CXL1.1. It needs the same
functionality for CXL PCIe ports operating in virtual hierarchy (VH)
mode.[1]

CXL and PCIe protocol error handling have different requirements that
necessitate a separate handling path. The AER service driver may try to
recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not
suitable for CXL PCIe port devices because of potential for system memory
corruption. Instead, CXL protocol error handling must use a kernel panic
in the case of a fatal or non-fatal UCE. The AER driver's PCIe error
handling does not panic the kernel in response to a UCE.

Introduce a separate path for CXL protocol error handling in the AER
service driver. This will allow CXL protocol errors to use CXL specific
handling instead of PCIe handling. Add the CXL specific changes without
affecting or adding functionality in the PCIe handling.

Make this update alongside the existing downstream port RCH error handling
logic, extending support to CXL PCIe ports in VH mode.

is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel
config. Update is_internal_error()'s function declaration such that it is
always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled
or disabled.

The uncorrectable error (UCE) handling will be added in a future patch.

[1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and
Upstream Switch Ports

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/pci/pcie/aer.c | 59 ++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 20 deletions(-)

Comments

Lukas Wunner Nov. 14, 2024, 4:44 p.m. UTC | #1 
On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
> @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
> -	pci_aer_handle_error(dev, info);
> +	if (is_internal_error(info) && handles_cxl_errors(dev))
> +		cxl_handle_error(dev, info);
> +	else
> +		pci_aer_handle_error(dev, info);
> +
>  	pci_dev_put(dev);
>  }

If you just do this at the top of cxl_handle_error()...

	if (!is_internal_error(info))
		return;

...you avoid the need to move is_internal_error() around and the
patch becomes simpler and easier to review.


> The AER service driver supports handling downstream port protocol errors in
> restricted CXL host (RCH) mode also known as CXL1.1. It needs the same
> functionality for CXL PCIe ports operating in virtual hierarchy (VH)
> mode.[1]

This is somewhat minor but by convention, patches in the PCI subsystem
adhere to spec language and capitalization, e.g. "Downstream Port"
instead of "downstream port".  Makes it easier to connect the commit
message or code comments to the spec.  So maybe you want to consider
that if/when respinning.

Thanks,

Lukas
Bowman, Terry Nov. 14, 2024, 6:41 p.m. UTC | #2 
Hi Lukas,

I added comments below.

On 11/14/2024 10:44 AM, Lukas Wunner wrote:
> On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
>> @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
>> -	pci_aer_handle_error(dev, info);
>> +	if (is_internal_error(info) && handles_cxl_errors(dev))
>> +		cxl_handle_error(dev, info);
>> +	else
>> +		pci_aer_handle_error(dev, info);
>> +
>>  	pci_dev_put(dev);
>>  }
> If you just do this at the top of cxl_handle_error()...
>
> 	if (!is_internal_error(info))
> 		return;
>
> ...you avoid the need to move is_internal_error() around and the
> patch becomes simpler and easier to review.

If is_internal_error()==0, then pci_aer_handle_error() should be called to process the PCIe error. Your suggestion would require returning a value from cxl_handle_error(). And then more "if" logic would be required for the cxl_handle_error() return value. Should both is_internal_error() and handles_cxl_errors()be moved into cxl_handle_error()? Would give this:

 static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
 {
-	cxl_handle_error(dev, info);
-	pci_aer_handle_error(dev, info);
+	if (!cxl_handle_error(dev, info))
+		pci_aer_handle_error(dev, info);
+
 	pci_dev_put(dev);
 }

>
>> The AER service driver supports handling downstream port protocol errors in
>> restricted CXL host (RCH) mode also known as CXL1.1. It needs the same
>> functionality for CXL PCIe ports operating in virtual hierarchy (VH)
>> mode.[1]
> This is somewhat minor but by convention, patches in the PCI subsystem
> adhere to spec language and capitalization, e.g. "Downstream Port"
> instead of "downstream port".  Makes it easier to connect the commit
> message or code comments to the spec.  So maybe you want to consider
> that if/when respinning.
>
> Thanks,
>
> Lukas
Thanks for pointing that out. I'll update as you suggest.

Regards,
Terry
Lukas Wunner Nov. 15, 2024, 8:51 a.m. UTC | #3 
On Thu, Nov 14, 2024 at 12:41:13PM -0600, Bowman, Terry wrote:
> On 11/14/2024 10:44 AM, Lukas Wunner wrote:
> > On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
> > > @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
> > > -	pci_aer_handle_error(dev, info);
> > > +	if (is_internal_error(info) && handles_cxl_errors(dev))
> > > +		cxl_handle_error(dev, info);
> > > +	else
> > > +		pci_aer_handle_error(dev, info);
> > > +
> > >  	pci_dev_put(dev);
> > >  }
> > 
> > If you just do this at the top of cxl_handle_error()...
> >
> > 	if (!is_internal_error(info))
> > 		return;
> >
> > ...you avoid the need to move is_internal_error() around and the
> > patch becomes simpler and easier to review.
> 
> If is_internal_error()==0, then pci_aer_handle_error() should be called
> to process the PCIe error.

You're absolutely right, I missed that, sorry for the noise.

Thanks,

Lukas
Bowman, Terry Nov. 15, 2024, 1:56 p.m. UTC | #4 
On 11/15/2024 2:51 AM, Lukas Wunner wrote:
> On Thu, Nov 14, 2024 at 12:41:13PM -0600, Bowman, Terry wrote:
>> On 11/14/2024 10:44 AM, Lukas Wunner wrote:
>>> On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
>>>> @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
>>>> -	pci_aer_handle_error(dev, info);
>>>> +	if (is_internal_error(info) && handles_cxl_errors(dev))
>>>> +		cxl_handle_error(dev, info);
>>>> +	else
>>>> +		pci_aer_handle_error(dev, info);
>>>> +
>>>>  	pci_dev_put(dev);
>>>>  }
>>> If you just do this at the top of cxl_handle_error()...
>>>
>>> 	if (!is_internal_error(info))
>>> 		return;
>>>
>>> ...you avoid the need to move is_internal_error() around and the
>>> patch becomes simpler and easier to review.
>> If is_internal_error()==0, then pci_aer_handle_error() should be called
>> to process the PCIe error.
> You're absolutely right, I missed that, sorry for the noise.
>
> Thanks,
>
> Lukas
Thanks for taking the time to review.

Regards,
Terry
Li Ming Nov. 15, 2024, 2:49 p.m. UTC | #5 
On 2024/11/15 2:41, Bowman, Terry wrote:
> Hi Lukas,
> 
> I added comments below.
> 
> On 11/14/2024 10:44 AM, Lukas Wunner wrote:
>> On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
>>> @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
>>> -	pci_aer_handle_error(dev, info);
>>> +	if (is_internal_error(info) && handles_cxl_errors(dev))
>>> +		cxl_handle_error(dev, info);
>>> +	else
>>> +		pci_aer_handle_error(dev, info);
>>> +
>>>   	pci_dev_put(dev);
>>>   }
>> If you just do this at the top of cxl_handle_error()...
>>
>> 	if (!is_internal_error(info))
>> 		return;
>>
>> ...you avoid the need to move is_internal_error() around and the
>> patch becomes simpler and easier to review.
> 
> If is_internal_error()==0, then pci_aer_handle_error() should be called to process the PCIe error. Your suggestion would require returning a value from cxl_handle_error(). And then more "if" logic would be required for the cxl_handle_error() return value. Should both is_internal_error() and handles_cxl_errors()be moved into cxl_handle_error()? Would give this:
> 
>   static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>   {
> -	cxl_handle_error(dev, info);
> -	pci_aer_handle_error(dev, info);
> +	if (!cxl_handle_error(dev, info))
> +		pci_aer_handle_error(dev, info);
> +
>   	pci_dev_put(dev);
>   }
> 

I think is_internal_error() can be moved into handles_cxl_errors(). 
handles_cxl_errors() helps to check if the error is a CXL error, avoid 
this detail which CXL error is an internal error in 
handle_error_source(). Like this:

    static void handle_error_source(struct pci_dev *dev, struct 
aer_err_info *info)
    {
  -	cxl_handle_error(dev, info);
  -	pci_aer_handle_error(dev, info);
  +	if (handles_cxl_errors(dev, info))
  +		cxl_handle_error(dev, info);
  +	else
  +		pci_aer_handle_error(dev, info);
  +
    	pci_dev_put(dev);
    }


Ming

>>
>>> The AER service driver supports handling downstream port protocol errors in
>>> restricted CXL host (RCH) mode also known as CXL1.1. It needs the same
>>> functionality for CXL PCIe ports operating in virtual hierarchy (VH)
>>> mode.[1]
>> This is somewhat minor but by convention, patches in the PCI subsystem
>> adhere to spec language and capitalization, e.g. "Downstream Port"
>> instead of "downstream port".  Makes it easier to connect the commit
>> message or code comments to the spec.  So maybe you want to consider
>> that if/when respinning.
>>
>> Thanks,
>>
>> Lukas
> Thanks for pointing that out. I'll update as you suggest.
> 
> Regards,
> Terry
>
Bowman, Terry Nov. 15, 2024, 7:46 p.m. UTC | #6 
On 11/15/2024 8:49 AM, Li Ming wrote:
>
> On 2024/11/15 2:41, Bowman, Terry wrote:
>> Hi Lukas,
>>
>> I added comments below.
>>
>> On 11/14/2024 10:44 AM, Lukas Wunner wrote:
>>> On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
>>>> @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
>>>> -	pci_aer_handle_error(dev, info);
>>>> +	if (is_internal_error(info) && handles_cxl_errors(dev))
>>>> +		cxl_handle_error(dev, info);
>>>> +	else
>>>> +		pci_aer_handle_error(dev, info);
>>>> +
>>>>   	pci_dev_put(dev);
>>>>   }
>>> If you just do this at the top of cxl_handle_error()...
>>>
>>> 	if (!is_internal_error(info))
>>> 		return;
>>>
>>> ...you avoid the need to move is_internal_error() around and the
>>> patch becomes simpler and easier to review.
>> If is_internal_error()==0, then pci_aer_handle_error() should be called to process the PCIe error. Your suggestion would require returning a value from cxl_handle_error(). And then more "if" logic would be required for the cxl_handle_error() return value. Should both is_internal_error() and handles_cxl_errors()be moved into cxl_handle_error()? Would give this:
>>
>>   static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>>   {
>> -	cxl_handle_error(dev, info);
>> -	pci_aer_handle_error(dev, info);
>> +	if (!cxl_handle_error(dev, info))
>> +		pci_aer_handle_error(dev, info);
>> +
>>   	pci_dev_put(dev);
>>   }
>>

We could do that. And with that change it might need handles_cxl_errors() renamed
to something more correct, like handle_cxl_error()?

Regards,
Terry
Li Ming Nov. 17, 2024, 7:38 a.m. UTC | #7 
On 2024/11/16 3:46, Bowman, Terry wrote:
> 
> 
> On 11/15/2024 8:49 AM, Li Ming wrote:
>>
>> On 2024/11/15 2:41, Bowman, Terry wrote:
>>> Hi Lukas,
>>>
>>> I added comments below.
>>>
>>> On 11/14/2024 10:44 AM, Lukas Wunner wrote:
>>>> On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
>>>>> @@ -1115,8 +1131,11 @@ 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_handle_error(dev, info);
>>>>> -	pci_aer_handle_error(dev, info);
>>>>> +	if (is_internal_error(info) && handles_cxl_errors(dev))
>>>>> +		cxl_handle_error(dev, info);
>>>>> +	else
>>>>> +		pci_aer_handle_error(dev, info);
>>>>> +
>>>>>    	pci_dev_put(dev);
>>>>>    }
>>>> If you just do this at the top of cxl_handle_error()...
>>>>
>>>> 	if (!is_internal_error(info))
>>>> 		return;
>>>>
>>>> ...you avoid the need to move is_internal_error() around and the
>>>> patch becomes simpler and easier to review.
>>> If is_internal_error()==0, then pci_aer_handle_error() should be called to process the PCIe error. Your suggestion would require returning a value from cxl_handle_error(). And then more "if" logic would be required for the cxl_handle_error() return value. Should both is_internal_error() and handles_cxl_errors()be moved into cxl_handle_error()? Would give this:
>>>
>>>    static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>>>    {
>>> -	cxl_handle_error(dev, info);
>>> -	pci_aer_handle_error(dev, info);
>>> +	if (!cxl_handle_error(dev, info))
>>> +		pci_aer_handle_error(dev, info);
>>> +
>>>    	pci_dev_put(dev);
>>>    }
>>>
> 
> We could do that. And with that change it might need handles_cxl_errors() renamed
> to something more correct, like handle_cxl_error()?

Yes, the name you mentioned is better.

Ming

> 
> Regards,
> Terry
> 
>
diff mbox series

Patch

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 53e9a11f6c0f..1d3e5b929661 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -941,8 +941,15 @@  static bool find_source_device(struct pci_dev *parent,
 	return true;
 }
 
-#ifdef CONFIG_PCIEAER_CXL
+static bool is_internal_error(struct aer_err_info *info)
+{
+	if (info->severity == AER_CORRECTABLE)
+		return info->status & PCI_ERR_COR_INTERNAL;
 
+	return info->status & PCI_ERR_UNC_INTN;
+}
+
+#ifdef CONFIG_PCIEAER_CXL
 /**
  * pci_aer_unmask_internal_errors - unmask internal errors
  * @dev: pointer to the pcie_dev data structure
@@ -994,14 +1001,6 @@  static bool cxl_error_is_native(struct pci_dev *dev)
 	return (pcie_ports_native || host->native_aer);
 }
 
-static bool is_internal_error(struct aer_err_info *info)
-{
-	if (info->severity == AER_CORRECTABLE)
-		return info->status & PCI_ERR_COR_INTERNAL;
-
-	return info->status & PCI_ERR_UNC_INTN;
-}
-
 static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
 {
 	struct aer_err_info *info = (struct aer_err_info *)data;
@@ -1033,14 +1032,23 @@  static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
 
 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.
-	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC &&
-	    is_internal_error(info))
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
 		pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info);
+
+	if (info->severity == AER_CORRECTABLE) {
+		struct pci_driver *pdrv = dev->driver;
+		int aer = dev->aer_cap;
+
+		if (aer)
+			pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS,
+					       info->status);
+
+		if (pdrv && pdrv->cxl_err_handler &&
+		    pdrv->cxl_err_handler->cor_error_detected)
+			pdrv->cxl_err_handler->cor_error_detected(dev);
+
+		pcie_clear_device_status(dev);
+	}
 }
 
 static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
@@ -1058,9 +1066,13 @@  static bool handles_cxl_errors(struct pci_dev *dev)
 {
 	bool handles_cxl = false;
 
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC &&
-	    pcie_aer_is_native(dev))
+	if (!pcie_aer_is_native(dev))
+		return false;
+
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
 		pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl);
+	else
+		handles_cxl = pcie_is_cxl_port(dev);
 
 	return handles_cxl;
 }
@@ -1078,6 +1090,10 @@  static void cxl_enable_internal_errors(struct pci_dev *dev)
 static inline void cxl_enable_internal_errors(struct pci_dev *dev) { }
 static inline void cxl_handle_error(struct pci_dev *dev,
 				    struct aer_err_info *info) { }
+static bool handles_cxl_errors(struct pci_dev *dev)
+{
+	return false;
+}
 #endif
 
 /**
@@ -1115,8 +1131,11 @@  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_handle_error(dev, info);
-	pci_aer_handle_error(dev, info);
+	if (is_internal_error(info) && handles_cxl_errors(dev))
+		cxl_handle_error(dev, info);
+	else
+		pci_aer_handle_error(dev, info);
+
 	pci_dev_put(dev);
 }