diff mbox series

[v4,10/11] PCI/AER: Add optional logging callback for correctable error

Message ID 166974414546.1608150.4142682712102935008.stgit@djiang5-desk3.ch.intel.com
State New, archived
Headers show
Series cxl/pci: Add fundamental error handling | expand

Commit Message

Dave Jiang Nov. 29, 2022, 5:49 p.m. UTC
Some new devices such as CXL devices may want to record additional error
information on a corrected error. Add a callback to allow the PCI device
driver to do additional logging such as providing additional stats for user
space RAS monitoring.

For CXL device, this is actually a need due to CXL needing to write to the
device AER status register in order to clear the unmasked CEs.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/PCI/pci-error-recovery.rst |    7 +++++++
 drivers/pci/pcie/aer.c                   |    8 +++++++-
 include/linux/pci.h                      |    3 +++
 3 files changed, 17 insertions(+), 1 deletion(-)

Comments

Bjorn Helgaas Nov. 30, 2022, 7:45 p.m. UTC | #1
On Tue, Nov 29, 2022 at 10:49:05AM -0700, Dave Jiang wrote:
> Some new devices such as CXL devices may want to record additional error
> information on a corrected error. Add a callback to allow the PCI device
> driver to do additional logging such as providing additional stats for user
> space RAS monitoring.
> 
> For CXL device, this is actually a need due to CXL needing to write to the
> device AER status register in order to clear the unmasked CEs.

s/CE/correctable error/ since it's the first use and not common in
PCI-land.

"device AER status register" sounds like the PCIe AER Correctable
Error Status Register (PCIe r6.0, sec 7.8.4.5), but I think you mean
something else, maybe a CXL-specific register?

The PCIe core needs to own the AER one (PCI_ERR_COR_STATUS) so it can
coordinate ownership between firmware and Linux.

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  Documentation/PCI/pci-error-recovery.rst |    7 +++++++
>  drivers/pci/pcie/aer.c                   |    8 +++++++-
>  include/linux/pci.h                      |    3 +++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/PCI/pci-error-recovery.rst b/Documentation/PCI/pci-error-recovery.rst
> index 187f43a03200..690220255d5e 100644
> --- a/Documentation/PCI/pci-error-recovery.rst
> +++ b/Documentation/PCI/pci-error-recovery.rst
> @@ -83,6 +83,7 @@ This structure has the form::
>  		int (*mmio_enabled)(struct pci_dev *dev);
>  		int (*slot_reset)(struct pci_dev *dev);
>  		void (*resume)(struct pci_dev *dev);
> +		void (*cor_error_log)(struct pci_dev *dev);

I think I would remove "log" from the name because it suggests this
hook should *only* log, and you need to actually clear some status.
Maybe "cor_error_detected()" to be analogous to error_detected()?

>  	};
>  
>  The possible channel states are::
> @@ -422,5 +423,11 @@ That is, the recovery API only requires that:
>     - drivers/net/cxgb3
>     - drivers/net/s2io.c
>  
> +   The cor_error_log() callback is invoked in handle_error_source() when
> +   the error severity is "correctable". The callback is optional and allows
> +   additional logging to be done if desired. See example:
> +
> +   - drivers/cxl/pci.c
> +
>  The End
>  -------
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index e2d8a74f83c3..af1b5eecbb11 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -961,8 +961,14 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>  		if (aer)
>  			pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS,
>  					info->status);
> -		if (pcie_aer_is_native(dev))
> +		if (pcie_aer_is_native(dev)) {
> +			struct pci_driver *pdrv = dev->driver;
> +
> +			if (pdrv && pdrv->err_handler &&
> +			    pdrv->err_handler->cor_error_log)
> +				pdrv->err_handler->cor_error_log(dev);
>  			pcie_clear_device_status(dev);
> +		}
>  	} else if (info->severity == AER_NONFATAL)
>  		pcie_do_recovery(dev, pci_channel_io_normal, aer_root_reset);
>  	else if (info->severity == AER_FATAL)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 575849a100a3..54939b3426a9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -844,6 +844,9 @@ struct pci_error_handlers {
>  
>  	/* Device driver may resume normal operations */
>  	void (*resume)(struct pci_dev *dev);
> +
> +	/* Allow device driver to record more details of a correctable error */
> +	void (*cor_error_log)(struct pci_dev *dev);
>  };
>  
>  
> 
>
Dave Jiang Nov. 30, 2022, 9:37 p.m. UTC | #2
On 11/30/2022 12:45 PM, Bjorn Helgaas wrote:
> On Tue, Nov 29, 2022 at 10:49:05AM -0700, Dave Jiang wrote:
>> Some new devices such as CXL devices may want to record additional error
>> information on a corrected error. Add a callback to allow the PCI device
>> driver to do additional logging such as providing additional stats for user
>> space RAS monitoring.
>>
>> For CXL device, this is actually a need due to CXL needing to write to the
>> device AER status register in order to clear the unmasked CEs.
> 
> s/CE/correctable error/ since it's the first use and not common in
> PCI-land.
> 

Ok

> "device AER status register" sounds like the PCIe AER Correctable
> Error Status Register (PCIe r6.0, sec 7.8.4.5), but I think you mean
> something else, maybe a CXL-specific register?

Yes. It's part of the CXL device RAS structure. I'll add more details.

> 
> The PCIe core needs to own the AER one (PCI_ERR_COR_STATUS) so it can
> coordinate ownership between firmware and Linux.
> 
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thank you!

> 
>> ---
>>   Documentation/PCI/pci-error-recovery.rst |    7 +++++++
>>   drivers/pci/pcie/aer.c                   |    8 +++++++-
>>   include/linux/pci.h                      |    3 +++
>>   3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/PCI/pci-error-recovery.rst b/Documentation/PCI/pci-error-recovery.rst
>> index 187f43a03200..690220255d5e 100644
>> --- a/Documentation/PCI/pci-error-recovery.rst
>> +++ b/Documentation/PCI/pci-error-recovery.rst
>> @@ -83,6 +83,7 @@ This structure has the form::
>>   		int (*mmio_enabled)(struct pci_dev *dev);
>>   		int (*slot_reset)(struct pci_dev *dev);
>>   		void (*resume)(struct pci_dev *dev);
>> +		void (*cor_error_log)(struct pci_dev *dev);
> 
> I think I would remove "log" from the name because it suggests this
> hook should *only* log, and you need to actually clear some status.
> Maybe "cor_error_detected()" to be analogous to error_detected()?

Ok I'll change.

> 
>>   	};
>>   
>>   The possible channel states are::
>> @@ -422,5 +423,11 @@ That is, the recovery API only requires that:
>>      - drivers/net/cxgb3
>>      - drivers/net/s2io.c
>>   
>> +   The cor_error_log() callback is invoked in handle_error_source() when
>> +   the error severity is "correctable". The callback is optional and allows
>> +   additional logging to be done if desired. See example:
>> +
>> +   - drivers/cxl/pci.c
>> +
>>   The End
>>   -------
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index e2d8a74f83c3..af1b5eecbb11 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -961,8 +961,14 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>>   		if (aer)
>>   			pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS,
>>   					info->status);
>> -		if (pcie_aer_is_native(dev))
>> +		if (pcie_aer_is_native(dev)) {
>> +			struct pci_driver *pdrv = dev->driver;
>> +
>> +			if (pdrv && pdrv->err_handler &&
>> +			    pdrv->err_handler->cor_error_log)
>> +				pdrv->err_handler->cor_error_log(dev);
>>   			pcie_clear_device_status(dev);
>> +		}
>>   	} else if (info->severity == AER_NONFATAL)
>>   		pcie_do_recovery(dev, pci_channel_io_normal, aer_root_reset);
>>   	else if (info->severity == AER_FATAL)
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 575849a100a3..54939b3426a9 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -844,6 +844,9 @@ struct pci_error_handlers {
>>   
>>   	/* Device driver may resume normal operations */
>>   	void (*resume)(struct pci_dev *dev);
>> +
>> +	/* Allow device driver to record more details of a correctable error */
>> +	void (*cor_error_log)(struct pci_dev *dev);
>>   };
>>   
>>   
>>
>>
diff mbox series

Patch

diff --git a/Documentation/PCI/pci-error-recovery.rst b/Documentation/PCI/pci-error-recovery.rst
index 187f43a03200..690220255d5e 100644
--- a/Documentation/PCI/pci-error-recovery.rst
+++ b/Documentation/PCI/pci-error-recovery.rst
@@ -83,6 +83,7 @@  This structure has the form::
 		int (*mmio_enabled)(struct pci_dev *dev);
 		int (*slot_reset)(struct pci_dev *dev);
 		void (*resume)(struct pci_dev *dev);
+		void (*cor_error_log)(struct pci_dev *dev);
 	};
 
 The possible channel states are::
@@ -422,5 +423,11 @@  That is, the recovery API only requires that:
    - drivers/net/cxgb3
    - drivers/net/s2io.c
 
+   The cor_error_log() callback is invoked in handle_error_source() when
+   the error severity is "correctable". The callback is optional and allows
+   additional logging to be done if desired. See example:
+
+   - drivers/cxl/pci.c
+
 The End
 -------
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index e2d8a74f83c3..af1b5eecbb11 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -961,8 +961,14 @@  static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
 		if (aer)
 			pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS,
 					info->status);
-		if (pcie_aer_is_native(dev))
+		if (pcie_aer_is_native(dev)) {
+			struct pci_driver *pdrv = dev->driver;
+
+			if (pdrv && pdrv->err_handler &&
+			    pdrv->err_handler->cor_error_log)
+				pdrv->err_handler->cor_error_log(dev);
 			pcie_clear_device_status(dev);
+		}
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_recovery(dev, pci_channel_io_normal, aer_root_reset);
 	else if (info->severity == AER_FATAL)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 575849a100a3..54939b3426a9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -844,6 +844,9 @@  struct pci_error_handlers {
 
 	/* Device driver may resume normal operations */
 	void (*resume)(struct pci_dev *dev);
+
+	/* Allow device driver to record more details of a correctable error */
+	void (*cor_error_log)(struct pci_dev *dev);
 };