Re: [PATCH v16 10/10] cxl: Enable CXL protocol error reporting

[Dan Williams]

Terry Bowman wrote:
> CXL protocol errors are not enabled for all CXL devices after boot. These
> must be enabled inorder to process CXL protocol errors.
> 
> Introduce cxl_unmask_proto_interrupts() to call pci_aer_unmask_internal_errors().
> pci_aer_unmask_internal_errors() expects the pdev->aer_cap is initialized.
> But, dev->aer_cap is not initialized for CXL Upstream Switch Ports and CXL
> Downstream Switch Ports. Initialize the dev->aer_cap if necessary. Enable AER
> correctable internal errors and uncorrectable internal errors for all CXL
> devices.
> 
> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Reviewed-by: Ben Cheatham <benjamin.cheatham@xxxxxxx>
> 
> ---
>  drivers/cxl/core/port.c |  2 ++
>  drivers/cxl/core/ras.c  | 22 ++++++++++++++++++++++
>  drivers/cxl/cxlpci.h    |  4 ++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 27271402915f..c33d58fb7264 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1852,6 +1852,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  
>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>  
> +			cxl_unmask_proto_interrupts(cxlmd->cxlds->dev);
> +

Why here? devm_cxl_port_ras_setup() will just redo it, right?

>  			/*
>  			 * If the endpoint already exists in the port's list,
>  			 * that's ok, it was added on a previous pass.
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index d6112b812c82..bfe6cb35154e 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -119,6 +119,24 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work)
>  }
>  static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn);
>  
> +void cxl_unmask_proto_interrupts(struct device *dev)
> +{
> +	if (!dev || !dev_is_pci(dev))
> +		return;
> +
> +	struct pci_dev *pdev __free(pci_dev_put) = pci_dev_get(to_pci_dev(dev));

Why does this need to hold a reference for a simple to_pci_dev() conversion?

> +
> +	if (!pdev->aer_cap) {
> +		pdev->aer_cap = pci_find_ext_capability(pdev,
> +							PCI_EXT_CAP_ID_ERR);
> +		if (!pdev->aer_cap)
> +			return;
> +	}
> +
> +	pci_aer_unmask_internal_errors(pdev);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_unmask_proto_interrupts, "CXL");

Why export? The only callers are internal to cxl_core.ko?

---
 drivers/cxl/cxlpci.h    | 4 ----
 drivers/cxl/core/port.c | 2 --
 drivers/cxl/core/ras.c  | 3 +--
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index b7cf9d6137b3..184a95e96ea9 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -82,7 +82,6 @@ void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport);
 pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
 					pci_channel_state_t error);
 void devm_cxl_port_ras_setup(struct cxl_port *port);
-void cxl_unmask_proto_interrupts(struct device *dev);
 #else
 static inline pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
 						      pci_channel_state_t state)
@@ -97,9 +96,6 @@ static inline void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
 static inline void devm_cxl_port_ras_setup(struct cxl_port *port)
 {
 }
-static inline void cxl_unmask_proto_interrupts(struct device *dev)
-{
-}
 #endif
 
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index c33d58fb7264..27271402915f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1852,8 +1852,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 
 			rc = cxl_add_ep(dport, &cxlmd->dev);
 
-			cxl_unmask_proto_interrupts(cxlmd->cxlds->dev);
-
 			/*
 			 * If the endpoint already exists in the port's list,
 			 * that's ok, it was added on a previous pass.
diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
index bfe6cb35154e..1f688110f0f4 100644
--- a/drivers/cxl/core/ras.c
+++ b/drivers/cxl/core/ras.c
@@ -119,7 +119,7 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work)
 }
 static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn);
 
-void cxl_unmask_proto_interrupts(struct device *dev)
+static void cxl_unmask_proto_interrupts(struct device *dev)
 {
 	if (!dev || !dev_is_pci(dev))
 		return;
@@ -135,7 +135,6 @@ void cxl_unmask_proto_interrupts(struct device *dev)
 
 	pci_aer_unmask_internal_errors(pdev);
 }
-EXPORT_SYMBOL_NS_GPL(cxl_unmask_proto_interrupts, "CXL");
 
 static void cxl_dport_map_ras(struct cxl_dport *dport)
 {
-- 
2.53.0