diff mbox series

[RFC,7/9] cxl/pci: Add atomic notifier callback for CXL PCIe port AER internal errors

Message ID 20240617200411.1426554-8-terry.bowman@amd.com
State New, archived
Headers show
Series Add RAS support for CXL root ports, CXL downstream switch ports, and CXL upstream switch ports | expand

Commit Message

Bowman, Terry June 17, 2024, 8:04 p.m. UTC
CXL root ports, CXL downstream switch ports, and CXL upstream switch
ports are bound to the PCIe port bus driver, portdrv. portdrv provides
an atomic notifier chain for reporting PCIe port device AER
correctable internal errors (CIE) and AER uncorrectable internal
errors (UIE).

CXL PCIe port devices use AER CIE/UIE to report CXL RAS.[1]

Add a cxl_pci atomic notification callback for handling the portdrv's
AER UIE/CIE notifications.

Register the atomic notification callback in the cxl_pci module's
load. Unregister the callback in the cxl_pci driver's unload.

Implement the callback to check if the device parameter is a valid
CXL PCIe port. If it is valid then make the notification callback call
__cxl_handle_cor_ras() or __cxl_handle_ras() depending on the AER
type.

[1] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and
             Upstream Switch Ports

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/cxl/core/core.h |  4 ++
 drivers/cxl/core/pci.c  | 97 ++++++++++++++++++++++++++++++++++++++---
 drivers/cxl/core/port.c |  6 +--
 drivers/cxl/cxl.h       |  5 +++
 drivers/cxl/cxlpci.h    |  2 +
 drivers/cxl/pci.c       | 19 +++++++-
 6 files changed, 123 insertions(+), 10 deletions(-)

Comments

Jonathan Cameron June 20, 2024, 1:09 p.m. UTC | #1
On Mon, 17 Jun 2024 15:04:09 -0500
Terry Bowman <terry.bowman@amd.com> wrote:

> CXL root ports, CXL downstream switch ports, and CXL upstream switch
> ports are bound to the PCIe port bus driver, portdrv. portdrv provides
> an atomic notifier chain for reporting PCIe port device AER
> correctable internal errors (CIE) and AER uncorrectable internal
> errors (UIE).
> 
> CXL PCIe port devices use AER CIE/UIE to report CXL RAS.[1]
> 
> Add a cxl_pci atomic notification callback for handling the portdrv's
> AER UIE/CIE notifications.
> 
> Register the atomic notification callback in the cxl_pci module's
> load. Unregister the callback in the cxl_pci driver's unload.
> 
> Implement the callback to check if the device parameter is a valid
> CXL PCIe port. If it is valid then make the notification callback call
> __cxl_handle_cor_ras() or __cxl_handle_ras() depending on the AER
> type.
> 
> [1] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and
>              Upstream Switch Ports
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Hi Terry,

Some comments inline.  Mostly this comes down to earlier question of whether
this notifier should be registered per device or globally. 
I think I still prefer per device, but attaching the handler will be trickier
and I'm guessing there may be some locking/lifetime issues doing that which
are avoided by a global notifier.

Jonathan

> ---
>  drivers/cxl/core/core.h |  4 ++
>  drivers/cxl/core/pci.c  | 97 ++++++++++++++++++++++++++++++++++++++---
>  drivers/cxl/core/port.c |  6 +--
>  drivers/cxl/cxl.h       |  5 +++
>  drivers/cxl/cxlpci.h    |  2 +
>  drivers/cxl/pci.c       | 19 +++++++-
>  6 files changed, 123 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index bc5a95665aa0..69bef1db6ee0 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -94,4 +94,8 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>  				       enum access_coordinate_class access);
>  bool cxl_need_node_perf_attrs_update(int nid);
>  
> +struct cxl_dport *find_dport(struct cxl_port *port, int id);
> +struct cxl_port *find_cxl_port(struct device *dport_dev,
> +			       struct cxl_dport **dport);
> +
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 59a317ab84bb..e630eccb733d 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -689,7 +689,6 @@ EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>  static void __cxl_handle_cor_ras(struct device *dev,
>  				 void __iomem *ras_base)
>  {
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	void __iomem *addr;
>  	u32 status;
>  
> @@ -698,10 +697,17 @@ static void __cxl_handle_cor_ras(struct device *dev,
>  
>  	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>  	status = readl(addr);
> -	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
> -		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> +

Blank line probably not wanted as we want to group the status
check with the read (it's kind of an error check).

> +	if (!(status & CXL_RAS_CORRECTABLE_STATUS_MASK))
> +		return;
> +
> +	writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> +	if (is_cxl_memdev(dev)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +
>  		trace_cxl_aer_correctable_error(cxlmd, status);
As below - don't bother with local cxlmd variable.

> -	}
> +	} else if (dev_is_pci(dev))
> +		trace_cxl_port_aer_correctable_error(dev, status);
>  }
>  
>  static void cxl_handle_endpoint_cor_ras(struct cxl_dev_state *cxlds)
> @@ -733,7 +739,6 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
>  static bool __cxl_handle_ras(struct device *dev,
>  			     void __iomem *ras_base)
>  {
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	u32 hl[CXL_HEADERLOG_SIZE_U32];
>  	void __iomem *addr;
>  	u32 status;
> @@ -759,7 +764,13 @@ static bool __cxl_handle_ras(struct device *dev,
>  	}
>  
>  	header_log_copy(ras_base, hl);
> -	trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
> +	if (is_cxl_memdev(dev)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
Just use this inline.
		trace_cxl_aer_uncorrectable_error(to_cxl_memdev(dev),
						  status, fe, hl);

> +
> +		trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
> +	} else if (dev_is_pci(dev))
> +		trace_cxl_port_aer_uncorrectable_error(dev, status);

As before, why no fe or hl?  I'm sure I'm missing some spec subtlty
but a comment would help me and others avoid that.

> +
>  	writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>  
>  	return true;
> @@ -882,6 +893,80 @@ static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
>  	return __cxl_handle_ras(&cxlds->cxlmd->dev, dport->regs.ras);
>  }
>  
> +static int match_uport(struct device *dev, void *data)
> +{
> +	struct device *uport_dev = (struct device *)data;
> +	struct cxl_port *port;
> +
> +	if (!is_cxl_port(dev))
> +		return 0;
> +
> +	port = to_cxl_port(dev);
> +
> +	return (port->uport_dev == uport_dev);
() doesn't add much so I'd drop them.

> +}
> +
> +static struct cxl_port *pci_to_cxl_uport(struct pci_dev *pdev)
> +{
> +	struct cxl_dport *dport;
> +	struct device *port_dev;
> +	struct cxl_port *port;
> +
> +	port = find_cxl_port(pdev->dev.parent, &dport);
> +	if (!port)
> +		return NULL;
> +	put_device(&port->dev);
I'm confused on the lifetimes. Doesn't it make more sense
to hold this until after you've stopped using it? So move the
put after device_find_child(...)

> +
> +	port_dev = device_find_child(&port->dev, &pdev->dev, match_uport);
> +	if (!port_dev)
> +		return NULL;
> +	put_device(port_dev);
> +
> +	port = to_cxl_port(port_dev);
> +
> +	return port;

	return to_cxl_port(port_dev);

> +}
> +
> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
> +{
> +	void __iomem *ras_base = NULL;
Don't initialize and...
> +
> +	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
> +	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> +		struct cxl_dport *dport;
> +
> +		find_cxl_port(&pdev->dev, &dport);
> +		ras_base = dport ? dport->regs.ras : NULL;
		if (dport)
			return dport->regs.ras;
> +	} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM) {
> +		struct cxl_port *port = pci_to_cxl_uport(pdev);
> +
> +		ras_base = port ? port->regs.ras : NULL;
		if (port)
			return port->regs.ras;
> +	}
return NULL;
> +
> +	return ras_base;
> +}
> +
> +int port_internal_err_cb(struct notifier_block *unused,
> +			 unsigned long event, void *ptr)
> +{
> +	struct pci_dev *pdev = (struct pci_dev *)ptr;

I think you can use this notifier for other types of device in future?
If it's going to be global definitely want to check here that we
actually have a CXL port of some type.

It may be that via the various calls any non CXL device
will result in a safe error. However that's not obvious, so an
upfront check makes sense (or a per device notifier registration!)

> +	void __iomem *ras_base;
> +
> +	if (!pdev)
> +		return 0;
> +
> +	if (event == AER_CORRECTABLE) {
> +		ras_base = cxl_pci_port_ras(pdev);
> +		__cxl_handle_cor_ras(&pdev->dev, ras_base);

as below - one line should be fine for this.

> +	} else if ((event == AER_FATAL) || (event == AER_NONFATAL)) {
> +		ras_base = cxl_pci_port_ras(pdev);
> +		__cxl_handle_ras(&pdev->dev, ras_base);

		__cxl_handle_ras(&pdev->dev, cxl_pci_port_ras(pdev));

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(port_internal_err_cb, CXL);
> +
>  /*
>   * Copy the AER capability registers using 32 bit read accesses.
>   * This is necessary because RCRB AER capability is MMIO mapped. Clear the
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 887ed6e358fb..d0f95c965ab4 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1027,7 +1027,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
>  }
>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL);
>  
> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> +struct cxl_dport *find_dport(struct cxl_port *port, int id)
>  {
>  	struct cxl_dport *dport;
>  	unsigned long index;
> @@ -1336,8 +1336,8 @@ static struct cxl_port *__find_cxl_port(struct cxl_find_port_ctx *ctx)
>  	return NULL;
>  }
>  
> -static struct cxl_port *find_cxl_port(struct device *dport_dev,
> -				      struct cxl_dport **dport)
> +struct cxl_port *find_cxl_port(struct device *dport_dev,
> +			       struct cxl_dport **dport)
>  {
>  	struct cxl_find_port_ctx ctx = {
>  		.dport_dev = dport_dev,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 7cee678fdb75..04725344393b 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -11,6 +11,7 @@
>  #include <linux/log2.h>
>  #include <linux/node.h>
>  #include <linux/io.h>
> +#include "../pci/pcie/portdrv.h"

Why add the include?  Maybe only needed in c files/

>  
>  /**
>   * DOC: cxl objects
> @@ -760,11 +761,15 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>  #ifdef CONFIG_PCIEAER_CXL
>  void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
>  void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
> +int port_internal_err_cb(struct notifier_block *unused,
> +			 unsigned long event, void *ptr);
>  #else
>  static inline void cxl_setup_parent_dport(struct device *host,
>  					  struct cxl_dport *dport) { }
>  static inline void cxl_setup_parent_uport(struct device *host,
>  					  struct cxl_port *port) { }
> +static inline int port_internal_err_cb(struct notifier_block *unused,
> +				unsigned long event, void *ptr) { return 0; }
>  #endif
>  
>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 93992a1c8eec..6044955e1c48 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -130,4 +130,6 @@ void read_cdat_data(struct cxl_port *port);
>  void cxl_cor_error_detected(struct pci_dev *pdev);
>  pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  				    pci_channel_state_t state);
> +int port_err_nb_cb(struct notifier_block *unused,
> +		   unsigned long event, void *ptr);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..f4183c5aea38 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -926,6 +926,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return rc;
>  }
>  
> +struct notifier_block port_internal_err_nb = {
> +	.notifier_call = port_internal_err_cb,
> +};
> +
>  static const struct pci_device_id cxl_mem_pci_tbl[] = {
>  	/* PCI class code for CXL.mem Type-3 Devices */
>  	{ PCI_DEVICE_CLASS((PCI_CLASS_MEMORY_CXL << 8 | CXL_MEMORY_PROGIF), ~0)},
> @@ -974,6 +978,19 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> -module_pci_driver(cxl_pci_driver);
> +static int __init cxl_pci_init(void)
> +{
> +	atomic_notifier_chain_register(&portdrv_aer_internal_err_chain, &port_internal_err_nb);

Long line that you can easily break.

> +	return pci_register_driver(&cxl_pci_driver);
> +}
> +module_init(cxl_pci_init);
> +
> +static void __exit cxl_pci_exit(void)
> +{
> +	atomic_notifier_chain_unregister(&portdrv_aer_internal_err_chain, &port_internal_err_nb);

Long line that you can easily break.

> +	pci_unregister_driver(&cxl_pci_driver);
> +}
> +module_exit(cxl_pci_exit);
> +
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(CXL);
Bowman, Terry June 24, 2024, 4:09 p.m. UTC | #2
Hi Jonathan,

I added repsonses inline below.

On 6/20/24 08:09, Jonathan Cameron wrote:
> On Mon, 17 Jun 2024 15:04:09 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
> 
>> CXL root ports, CXL downstream switch ports, and CXL upstream switch
>> ports are bound to the PCIe port bus driver, portdrv. portdrv provides
>> an atomic notifier chain for reporting PCIe port device AER
>> correctable internal errors (CIE) and AER uncorrectable internal
>> errors (UIE).
>>
>> CXL PCIe port devices use AER CIE/UIE to report CXL RAS.[1]
>>
>> Add a cxl_pci atomic notification callback for handling the portdrv's
>> AER UIE/CIE notifications.
>>
>> Register the atomic notification callback in the cxl_pci module's
>> load. Unregister the callback in the cxl_pci driver's unload.
>>
>> Implement the callback to check if the device parameter is a valid
>> CXL PCIe port. If it is valid then make the notification callback call
>> __cxl_handle_cor_ras() or __cxl_handle_ras() depending on the AER
>> type.
>>
>> [1] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and
>>              Upstream Switch Ports
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Hi Terry,
> 
> Some comments inline.  Mostly this comes down to earlier question of whether
> this notifier should be registered per device or globally. 
> I think I still prefer per device, but attaching the handler will be trickier
> and I'm guessing there may be some locking/lifetime issues doing that which
> are avoided by a global notifier.
> 
> Jonathan
> 

I agree on the per-device notifier.

>> ---
>>  drivers/cxl/core/core.h |  4 ++
>>  drivers/cxl/core/pci.c  | 97 ++++++++++++++++++++++++++++++++++++++---
>>  drivers/cxl/core/port.c |  6 +--
>>  drivers/cxl/cxl.h       |  5 +++
>>  drivers/cxl/cxlpci.h    |  2 +
>>  drivers/cxl/pci.c       | 19 +++++++-
>>  6 files changed, 123 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index bc5a95665aa0..69bef1db6ee0 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -94,4 +94,8 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>>  				       enum access_coordinate_class access);
>>  bool cxl_need_node_perf_attrs_update(int nid);
>>  
>> +struct cxl_dport *find_dport(struct cxl_port *port, int id);
>> +struct cxl_port *find_cxl_port(struct device *dport_dev,
>> +			       struct cxl_dport **dport);
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 59a317ab84bb..e630eccb733d 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -689,7 +689,6 @@ EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>>  static void __cxl_handle_cor_ras(struct device *dev,
>>  				 void __iomem *ras_base)
>>  {
>> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>  	void __iomem *addr;
>>  	u32 status;
>>  
>> @@ -698,10 +697,17 @@ static void __cxl_handle_cor_ras(struct device *dev,
>>  
>>  	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>>  	status = readl(addr);
>> -	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
>> -		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>> +
> 
> Blank line probably not wanted as we want to group the status
> check with the read (it's kind of an error check).
> 

Ok.

>> +	if (!(status & CXL_RAS_CORRECTABLE_STATUS_MASK))
>> +		return;
>> +
>> +	writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>> +	if (is_cxl_memdev(dev)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +
>>  		trace_cxl_aer_correctable_error(cxlmd, status);
> As below - don't bother with local cxlmd variable.
> 

Ok.

>> -	}
>> +	} else if (dev_is_pci(dev))
>> +		trace_cxl_port_aer_correctable_error(dev, status);
>>  }
>>  
>>  static void cxl_handle_endpoint_cor_ras(struct cxl_dev_state *cxlds)
>> @@ -733,7 +739,6 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
>>  static bool __cxl_handle_ras(struct device *dev,
>>  			     void __iomem *ras_base)
>>  {
>> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>  	u32 hl[CXL_HEADERLOG_SIZE_U32];
>>  	void __iomem *addr;
>>  	u32 status;
>> @@ -759,7 +764,13 @@ static bool __cxl_handle_ras(struct device *dev,
>>  	}
>>  
>>  	header_log_copy(ras_base, hl);
>> -	trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
>> +	if (is_cxl_memdev(dev)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> Just use this inline.
> 		trace_cxl_aer_uncorrectable_error(to_cxl_memdev(dev),
> 						  status, fe, hl);
> 
>> +
>> +		trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
>> +	} else if (dev_is_pci(dev))
>> +		trace_cxl_port_aer_uncorrectable_error(dev, status);
> 
> As before, why no fe or hl?  I'm sure I'm missing some spec subtlty
> but a comment would help me and others avoid that.
> 

Adding the fe and hl on the list to be added. No, you're spot on. 

>> +
>>  	writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>>  
>>  	return true;
>> @@ -882,6 +893,80 @@ static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
>>  	return __cxl_handle_ras(&cxlds->cxlmd->dev, dport->regs.ras);
>>  }
>>  
>> +static int match_uport(struct device *dev, void *data)
>> +{
>> +	struct device *uport_dev = (struct device *)data;
>> +	struct cxl_port *port;
>> +
>> +	if (!is_cxl_port(dev))
>> +		return 0;
>> +
>> +	port = to_cxl_port(dev);
>> +
>> +	return (port->uport_dev == uport_dev);
> () doesn't add much so I'd drop them.
> 
>> +}
>> +
>> +static struct cxl_port *pci_to_cxl_uport(struct pci_dev *pdev)
>> +{
>> +	struct cxl_dport *dport;
>> +	struct device *port_dev;
>> +	struct cxl_port *port;
>> +
>> +	port = find_cxl_port(pdev->dev.parent, &dport);
>> +	if (!port)
>> +		return NULL;
>> +	put_device(&port->dev);
> I'm confused on the lifetimes. Doesn't it make more sense
> to hold this until after you've stopped using it? So move the
> put after device_find_child(...)
> 

Ok.

>> +
>> +	port_dev = device_find_child(&port->dev, &pdev->dev, match_uport);
>> +	if (!port_dev)
>> +		return NULL;
>> +	put_device(port_dev);
>> +
>> +	port = to_cxl_port(port_dev);
>> +
>> +	return port;
> 
> 	return to_cxl_port(port_dev);
> 
>> +}
>> +
>> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
>> +{
>> +	void __iomem *ras_base = NULL;
> Don't initialize and...

There is possibility the incorrect PCI type is passed and this is intended to
return NULL for these cases. Should ras_base not be preinitialized in 
that for the scenario?

>> +
>> +	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
>> +	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
>> +		struct cxl_dport *dport;
>> +
>> +		find_cxl_port(&pdev->dev, &dport);
>> +		ras_base = dport ? dport->regs.ras : NULL;
> 		if (dport)
> 			return dport->regs.ras;
>> +	} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM) {
>> +		struct cxl_port *port = pci_to_cxl_uport(pdev);
>> +
>> +		ras_base = port ? port->regs.ras : NULL;
> 		if (port)
> 			return port->regs.ras;
>> +	}
> return NULL;
>> +
>> +	return ras_base;
>> +}
>> +
>> +int port_internal_err_cb(struct notifier_block *unused,
>> +			 unsigned long event, void *ptr)
>> +{
>> +	struct pci_dev *pdev = (struct pci_dev *)ptr;
> 
> I think you can use this notifier for other types of device in future?
> If it's going to be global definitely want to check here that we
> actually have a CXL port of some type.
> 
> It may be that via the various calls any non CXL device
> will result in a safe error. However that's not obvious, so an
> upfront check makes sense (or a per device notifier registration!)
> 

cxl_pci_port_ras() performs PCIe type check and sets RAS base to NULL if 
the type is not a port.

>> +	void __iomem *ras_base;
>> +
>> +	if (!pdev)
>> +		return 0;
>> +
>> +	if (event == AER_CORRECTABLE) {
>> +		ras_base = cxl_pci_port_ras(pdev);
>> +		__cxl_handle_cor_ras(&pdev->dev, ras_base);
> 
> as below - one line should be fine for this.
> 
>> +	} else if ((event == AER_FATAL) || (event == AER_NONFATAL)) {
>> +		ras_base = cxl_pci_port_ras(pdev);
>> +		__cxl_handle_ras(&pdev->dev, ras_base);
> 
> 		__cxl_handle_ras(&pdev->dev, cxl_pci_port_ras(pdev));
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(port_internal_err_cb, CXL);
>> +
>>  /*
>>   * Copy the AER capability registers using 32 bit read accesses.
>>   * This is necessary because RCRB AER capability is MMIO mapped. Clear the
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 887ed6e358fb..d0f95c965ab4 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1027,7 +1027,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL);
>>  
>> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
>> +struct cxl_dport *find_dport(struct cxl_port *port, int id)
>>  {
>>  	struct cxl_dport *dport;
>>  	unsigned long index;
>> @@ -1336,8 +1336,8 @@ static struct cxl_port *__find_cxl_port(struct cxl_find_port_ctx *ctx)
>>  	return NULL;
>>  }
>>  
>> -static struct cxl_port *find_cxl_port(struct device *dport_dev,
>> -				      struct cxl_dport **dport)
>> +struct cxl_port *find_cxl_port(struct device *dport_dev,
>> +			       struct cxl_dport **dport)
>>  {
>>  	struct cxl_find_port_ctx ctx = {
>>  		.dport_dev = dport_dev,
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 7cee678fdb75..04725344393b 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -11,6 +11,7 @@
>>  #include <linux/log2.h>
>>  #include <linux/node.h>
>>  #include <linux/io.h>
>> +#include "../pci/pcie/portdrv.h"
> 
> Why add the include?  Maybe only needed in c files/
> 

Ok

>>  
>>  /**
>>   * DOC: cxl objects
>> @@ -760,11 +761,15 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>>  #ifdef CONFIG_PCIEAER_CXL
>>  void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
>>  void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
>> +int port_internal_err_cb(struct notifier_block *unused,
>> +			 unsigned long event, void *ptr);
>>  #else
>>  static inline void cxl_setup_parent_dport(struct device *host,
>>  					  struct cxl_dport *dport) { }
>>  static inline void cxl_setup_parent_uport(struct device *host,
>>  					  struct cxl_port *port) { }
>> +static inline int port_internal_err_cb(struct notifier_block *unused,
>> +				unsigned long event, void *ptr) { return 0; }
>>  #endif
>>  
>>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
>> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
>> index 93992a1c8eec..6044955e1c48 100644
>> --- a/drivers/cxl/cxlpci.h
>> +++ b/drivers/cxl/cxlpci.h
>> @@ -130,4 +130,6 @@ void read_cdat_data(struct cxl_port *port);
>>  void cxl_cor_error_detected(struct pci_dev *pdev);
>>  pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>>  				    pci_channel_state_t state);
>> +int port_err_nb_cb(struct notifier_block *unused,
>> +		   unsigned long event, void *ptr);
>>  #endif /* __CXL_PCI_H__ */
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 2ff361e756d6..f4183c5aea38 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -926,6 +926,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  	return rc;
>>  }
>>  
>> +struct notifier_block port_internal_err_nb = {
>> +	.notifier_call = port_internal_err_cb,
>> +};
>> +
>>  static const struct pci_device_id cxl_mem_pci_tbl[] = {
>>  	/* PCI class code for CXL.mem Type-3 Devices */
>>  	{ PCI_DEVICE_CLASS((PCI_CLASS_MEMORY_CXL << 8 | CXL_MEMORY_PROGIF), ~0)},
>> @@ -974,6 +978,19 @@ static struct pci_driver cxl_pci_driver = {
>>  	},
>>  };
>>  
>> -module_pci_driver(cxl_pci_driver);
>> +static int __init cxl_pci_init(void)
>> +{
>> +	atomic_notifier_chain_register(&portdrv_aer_internal_err_chain, &port_internal_err_nb);
> 
> Long line that you can easily break.
> 
>> +	return pci_register_driver(&cxl_pci_driver);
>> +}
>> +module_init(cxl_pci_init);
>> +
>> +static void __exit cxl_pci_exit(void)
>> +{
>> +	atomic_notifier_chain_unregister(&portdrv_aer_internal_err_chain, &port_internal_err_nb);
> 
> Long line that you can easily break.
> 
>> +	pci_unregister_driver(&cxl_pci_driver);
>> +}
>> +module_exit(cxl_pci_exit);
>> +
>>  MODULE_LICENSE("GPL v2");
>>  MODULE_IMPORT_NS(CXL);
>
Li, Ming4 June 26, 2024, 6:22 a.m. UTC | #3
On 6/18/2024 4:04 AM, Terry Bowman wrote:
> CXL root ports, CXL downstream switch ports, and CXL upstream switch
> ports are bound to the PCIe port bus driver, portdrv. portdrv provides
> an atomic notifier chain for reporting PCIe port device AER
> correctable internal errors (CIE) and AER uncorrectable internal
> errors (UIE).
>
> CXL PCIe port devices use AER CIE/UIE to report CXL RAS.[1]
>
> Add a cxl_pci atomic notification callback for handling the portdrv's
> AER UIE/CIE notifications.
>
> Register the atomic notification callback in the cxl_pci module's
> load. Unregister the callback in the cxl_pci driver's unload.
>
> Implement the callback to check if the device parameter is a valid
> CXL PCIe port. If it is valid then make the notification callback call
> __cxl_handle_cor_ras() or __cxl_handle_ras() depending on the AER
> type.
>
> [1] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and
>              Upstream Switch Ports
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
>  drivers/cxl/core/core.h |  4 ++
>  drivers/cxl/core/pci.c  | 97 ++++++++++++++++++++++++++++++++++++++---
>  drivers/cxl/core/port.c |  6 +--
>  drivers/cxl/cxl.h       |  5 +++
>  drivers/cxl/cxlpci.h    |  2 +
>  drivers/cxl/pci.c       | 19 +++++++-
>  6 files changed, 123 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index bc5a95665aa0..69bef1db6ee0 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -94,4 +94,8 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>  				       enum access_coordinate_class access);
>  bool cxl_need_node_perf_attrs_update(int nid);
>  
> +struct cxl_dport *find_dport(struct cxl_port *port, int id);
> +struct cxl_port *find_cxl_port(struct device *dport_dev,
> +			       struct cxl_dport **dport);
> +
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 59a317ab84bb..e630eccb733d 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -689,7 +689,6 @@ EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>  static void __cxl_handle_cor_ras(struct device *dev,
>  				 void __iomem *ras_base)
>  {
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	void __iomem *addr;
>  	u32 status;
>  
> @@ -698,10 +697,17 @@ static void __cxl_handle_cor_ras(struct device *dev,
>  
>  	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>  	status = readl(addr);
> -	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
> -		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> +
> +	if (!(status & CXL_RAS_CORRECTABLE_STATUS_MASK))
> +		return;
> +
> +	writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> +	if (is_cxl_memdev(dev)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +
>  		trace_cxl_aer_correctable_error(cxlmd, status);
> -	}
> +	} else if (dev_is_pci(dev))
> +		trace_cxl_port_aer_correctable_error(dev, status);
>  }
>  
>  static void cxl_handle_endpoint_cor_ras(struct cxl_dev_state *cxlds)
> @@ -733,7 +739,6 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
>  static bool __cxl_handle_ras(struct device *dev,
>  			     void __iomem *ras_base)
>  {
> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	u32 hl[CXL_HEADERLOG_SIZE_U32];
>  	void __iomem *addr;
>  	u32 status;
> @@ -759,7 +764,13 @@ static bool __cxl_handle_ras(struct device *dev,
>  	}
>  
>  	header_log_copy(ras_base, hl);
> -	trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
> +	if (is_cxl_memdev(dev)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +
> +		trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
> +	} else if (dev_is_pci(dev))
> +		trace_cxl_port_aer_uncorrectable_error(dev, status);
> +
>  	writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>  
>  	return true;
> @@ -882,6 +893,80 @@ static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
>  	return __cxl_handle_ras(&cxlds->cxlmd->dev, dport->regs.ras);
>  }
>  
> +static int match_uport(struct device *dev, void *data)
> +{
> +	struct device *uport_dev = (struct device *)data;
> +	struct cxl_port *port;
> +
> +	if (!is_cxl_port(dev))
> +		return 0;
> +
> +	port = to_cxl_port(dev);
> +
> +	return (port->uport_dev == uport_dev);
> +}
> +
> +static struct cxl_port *pci_to_cxl_uport(struct pci_dev *pdev)
> +{
> +	struct cxl_dport *dport;
> +	struct device *port_dev;
> +	struct cxl_port *port;
> +
> +	port = find_cxl_port(pdev->dev.parent, &dport);
> +	if (!port)
> +		return NULL;
> +	put_device(&port->dev);
> +
> +	port_dev = device_find_child(&port->dev, &pdev->dev, match_uport);
> +	if (!port_dev)
> +		return NULL;

 seems like just a bus_find_device(&cxl_bus_type, NULL, &pdev->dev, match_uport) can replace these find_cxl_port() and device_find_child().


> +	put_device(port_dev);
> +
> +	port = to_cxl_port(port_dev);
> +
> +	return port;
> +}
> +
> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
> +{
> +	void __iomem *ras_base = NULL;
> +
> +	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
> +	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> +		struct cxl_dport *dport;
> +
> +		find_cxl_port(&pdev->dev, &dport);
> +		ras_base = dport ? dport->regs.ras : NULL;

Need put_device(&port->dev) after find_cxl_port(), use scope-based resource management __free() here should be better.


> +	} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM) {
> +		struct cxl_port *port = pci_to_cxl_uport(pdev);
> +
> +		ras_base = port ? port->regs.ras : NULL;
> +	}
> +
> +	return ras_base;
> +}
> +
> +int port_internal_err_cb(struct notifier_block *unused,
> +			 unsigned long event, void *ptr)
> +{
> +	struct pci_dev *pdev = (struct pci_dev *)ptr;
> +	void __iomem *ras_base;
> +
> +	if (!pdev)
> +		return 0;
> +
> +	if (event == AER_CORRECTABLE) {
> +		ras_base = cxl_pci_port_ras(pdev);
> +		__cxl_handle_cor_ras(&pdev->dev, ras_base);
> +	} else if ((event == AER_FATAL) || (event == AER_NONFATAL)) {
> +		ras_base = cxl_pci_port_ras(pdev);
> +		__cxl_handle_ras(&pdev->dev, ras_base);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(port_internal_err_cb, CXL);
> +
>  /*
>   * Copy the AER capability registers using 32 bit read accesses.
>   * This is necessary because RCRB AER capability is MMIO mapped. Clear the
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 887ed6e358fb..d0f95c965ab4 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1027,7 +1027,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
>  }
>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL);
>  
> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> +struct cxl_dport *find_dport(struct cxl_port *port, int id)
>  {
>  	struct cxl_dport *dport;
>  	unsigned long index;
> @@ -1336,8 +1336,8 @@ static struct cxl_port *__find_cxl_port(struct cxl_find_port_ctx *ctx)
>  	return NULL;
>  }
>  
> -static struct cxl_port *find_cxl_port(struct device *dport_dev,
> -				      struct cxl_dport **dport)
> +struct cxl_port *find_cxl_port(struct device *dport_dev,
> +			       struct cxl_dport **dport)
>  {
>  	struct cxl_find_port_ctx ctx = {
>  		.dport_dev = dport_dev,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 7cee678fdb75..04725344393b 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -11,6 +11,7 @@
>  #include <linux/log2.h>
>  #include <linux/node.h>
>  #include <linux/io.h>
> +#include "../pci/pcie/portdrv.h"
>  
>  /**
>   * DOC: cxl objects
> @@ -760,11 +761,15 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>  #ifdef CONFIG_PCIEAER_CXL
>  void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
>  void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
> +int port_internal_err_cb(struct notifier_block *unused,
> +			 unsigned long event, void *ptr);
>  #else
>  static inline void cxl_setup_parent_dport(struct device *host,
>  					  struct cxl_dport *dport) { }
>  static inline void cxl_setup_parent_uport(struct device *host,
>  					  struct cxl_port *port) { }
> +static inline int port_internal_err_cb(struct notifier_block *unused,
> +				unsigned long event, void *ptr) { return 0; }
>  #endif
>  
>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 93992a1c8eec..6044955e1c48 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -130,4 +130,6 @@ void read_cdat_data(struct cxl_port *port);
>  void cxl_cor_error_detected(struct pci_dev *pdev);
>  pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  				    pci_channel_state_t state);
> +int port_err_nb_cb(struct notifier_block *unused,
> +		   unsigned long event, void *ptr);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..f4183c5aea38 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -926,6 +926,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return rc;
>  }
>  
> +struct notifier_block port_internal_err_nb = {
> +	.notifier_call = port_internal_err_cb,
> +};
> +
>  static const struct pci_device_id cxl_mem_pci_tbl[] = {
>  	/* PCI class code for CXL.mem Type-3 Devices */
>  	{ PCI_DEVICE_CLASS((PCI_CLASS_MEMORY_CXL << 8 | CXL_MEMORY_PROGIF), ~0)},
> @@ -974,6 +978,19 @@ static struct pci_driver cxl_pci_driver = {
>  	},
>  };
>  
> -module_pci_driver(cxl_pci_driver);
> +static int __init cxl_pci_init(void)
> +{
> +	atomic_notifier_chain_register(&portdrv_aer_internal_err_chain, &port_internal_err_nb);
> +	return pci_register_driver(&cxl_pci_driver);
> +}
> +module_init(cxl_pci_init);
> +
> +static void __exit cxl_pci_exit(void)
> +{
> +	atomic_notifier_chain_unregister(&portdrv_aer_internal_err_chain, &port_internal_err_nb);
> +	pci_unregister_driver(&cxl_pci_driver);
> +}
> +module_exit(cxl_pci_exit);
> +
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(CXL);
Bowman, Terry June 26, 2024, 1:51 p.m. UTC | #4
On 6/26/24 01:22, Li, Ming4 wrote:
> On 6/18/2024 4:04 AM, Terry Bowman wrote:
>> CXL root ports, CXL downstream switch ports, and CXL upstream switch
>> ports are bound to the PCIe port bus driver, portdrv. portdrv provides
>> an atomic notifier chain for reporting PCIe port device AER
>> correctable internal errors (CIE) and AER uncorrectable internal
>> errors (UIE).
>>
>> CXL PCIe port devices use AER CIE/UIE to report CXL RAS.[1]
>>
>> Add a cxl_pci atomic notification callback for handling the portdrv's
>> AER UIE/CIE notifications.
>>
>> Register the atomic notification callback in the cxl_pci module's
>> load. Unregister the callback in the cxl_pci driver's unload.
>>
>> Implement the callback to check if the device parameter is a valid
>> CXL PCIe port. If it is valid then make the notification callback call
>> __cxl_handle_cor_ras() or __cxl_handle_ras() depending on the AER
>> type.
>>
>> [1] CXL3.1 - 12.2.2 CXL Root Ports, Downstream Switch Ports, and
>>              Upstream Switch Ports
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> ---
>>  drivers/cxl/core/core.h |  4 ++
>>  drivers/cxl/core/pci.c  | 97 ++++++++++++++++++++++++++++++++++++++---
>>  drivers/cxl/core/port.c |  6 +--
>>  drivers/cxl/cxl.h       |  5 +++
>>  drivers/cxl/cxlpci.h    |  2 +
>>  drivers/cxl/pci.c       | 19 +++++++-
>>  6 files changed, 123 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index bc5a95665aa0..69bef1db6ee0 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -94,4 +94,8 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
>>  				       enum access_coordinate_class access);
>>  bool cxl_need_node_perf_attrs_update(int nid);
>>  
>> +struct cxl_dport *find_dport(struct cxl_port *port, int id);
>> +struct cxl_port *find_cxl_port(struct device *dport_dev,
>> +			       struct cxl_dport **dport);
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 59a317ab84bb..e630eccb733d 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -689,7 +689,6 @@ EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>>  static void __cxl_handle_cor_ras(struct device *dev,
>>  				 void __iomem *ras_base)
>>  {
>> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>  	void __iomem *addr;
>>  	u32 status;
>>  
>> @@ -698,10 +697,17 @@ static void __cxl_handle_cor_ras(struct device *dev,
>>  
>>  	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>>  	status = readl(addr);
>> -	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
>> -		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>> +
>> +	if (!(status & CXL_RAS_CORRECTABLE_STATUS_MASK))
>> +		return;
>> +
>> +	writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>> +	if (is_cxl_memdev(dev)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +
>>  		trace_cxl_aer_correctable_error(cxlmd, status);
>> -	}
>> +	} else if (dev_is_pci(dev))
>> +		trace_cxl_port_aer_correctable_error(dev, status);
>>  }
>>  
>>  static void cxl_handle_endpoint_cor_ras(struct cxl_dev_state *cxlds)
>> @@ -733,7 +739,6 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
>>  static bool __cxl_handle_ras(struct device *dev,
>>  			     void __iomem *ras_base)
>>  {
>> -	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>  	u32 hl[CXL_HEADERLOG_SIZE_U32];
>>  	void __iomem *addr;
>>  	u32 status;
>> @@ -759,7 +764,13 @@ static bool __cxl_handle_ras(struct device *dev,
>>  	}
>>  
>>  	header_log_copy(ras_base, hl);
>> -	trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
>> +	if (is_cxl_memdev(dev)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +
>> +		trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
>> +	} else if (dev_is_pci(dev))
>> +		trace_cxl_port_aer_uncorrectable_error(dev, status);
>> +
>>  	writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>>  
>>  	return true;
>> @@ -882,6 +893,80 @@ static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
>>  	return __cxl_handle_ras(&cxlds->cxlmd->dev, dport->regs.ras);
>>  }
>>  
>> +static int match_uport(struct device *dev, void *data)
>> +{
>> +	struct device *uport_dev = (struct device *)data;
>> +	struct cxl_port *port;
>> +
>> +	if (!is_cxl_port(dev))
>> +		return 0;
>> +
>> +	port = to_cxl_port(dev);
>> +
>> +	return (port->uport_dev == uport_dev);
>> +}
>> +
>> +static struct cxl_port *pci_to_cxl_uport(struct pci_dev *pdev)
>> +{
>> +	struct cxl_dport *dport;
>> +	struct device *port_dev;
>> +	struct cxl_port *port;
>> +
>> +	port = find_cxl_port(pdev->dev.parent, &dport);
>> +	if (!port)
>> +		return NULL;
>> +	put_device(&port->dev);
>> +
>> +	port_dev = device_find_child(&port->dev, &pdev->dev, match_uport);
>> +	if (!port_dev)
>> +		return NULL;
> 
>  seems like just a bus_find_device(&cxl_bus_type, NULL, &pdev->dev, match_uport) can replace these find_cxl_port() and device_find_child().
> 
> 

That would be a good improvement/optimization. I'll look into making that change.

>> +	put_device(port_dev);
>> +
>> +	port = to_cxl_port(port_dev);
>> +
>> +	return port;
>> +}
>> +
>> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
>> +{
>> +	void __iomem *ras_base = NULL;
>> +
>> +	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
>> +	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
>> +		struct cxl_dport *dport;
>> +
>> +		find_cxl_port(&pdev->dev, &dport);
>> +		ras_base = dport ? dport->regs.ras : NULL;
> 
> Need put_device(&port->dev) after find_cxl_port(), use scope-based resource management __free() here should be better.
> 
> 

Thanks.

Regards,
Terry
Jonathan Cameron July 2, 2024, 3:58 p.m. UTC | #5
> >> +
> >> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
> >> +{
> >> +	void __iomem *ras_base = NULL;  
> > Don't initialize and...  
> 
> There is possibility the incorrect PCI type is passed and this is intended to
> return NULL for these cases. Should ras_base not be preinitialized in 
> that for the scenario?

From a code point of view at least, nope - just return NULL directly
give it's an error case.

> 
> >> +
> >> +	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
> >> +	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> >> +		struct cxl_dport *dport;
> >> +
> >> +		find_cxl_port(&pdev->dev, &dport);
> >> +		ras_base = dport ? dport->regs.ras : NULL;  
> > 		if (dport)
> > 			return dport->regs.ras;  
> >> +	} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM) {
> >> +		struct cxl_port *port = pci_to_cxl_uport(pdev);
> >> +
> >> +		ras_base = port ? port->regs.ras : NULL;  
> > 		if (port)
> > 			return port->regs.ras;  
> >> +	}  
> > return NULL;  

This is why you don't need to set ras_base.
If you get here it's always NULL.

> >> +
> >> +	return ras_base;
> >> +}
> >> +
> >> +int port_internal_err_cb(struct notifier_block *unused,
> >> +			 unsigned long event, void *ptr)
> >> +{
> >> +	struct pci_dev *pdev = (struct pci_dev *)ptr;  
> > 
> > I think you can use this notifier for other types of device in future?
> > If it's going to be global definitely want to check here that we
> > actually have a CXL port of some type.
> > 
> > It may be that via the various calls any non CXL device
> > will result in a safe error. However that's not obvious, so an
> > upfront check makes sense (or a per device notifier registration!)
> >   
> 
> cxl_pci_port_ras() performs PCIe type check and sets RAS base to NULL if
diff mbox series

Patch

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index bc5a95665aa0..69bef1db6ee0 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -94,4 +94,8 @@  int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
 				       enum access_coordinate_class access);
 bool cxl_need_node_perf_attrs_update(int nid);
 
+struct cxl_dport *find_dport(struct cxl_port *port, int id);
+struct cxl_port *find_cxl_port(struct device *dport_dev,
+			       struct cxl_dport **dport);
+
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 59a317ab84bb..e630eccb733d 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -689,7 +689,6 @@  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
 static void __cxl_handle_cor_ras(struct device *dev,
 				 void __iomem *ras_base)
 {
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	void __iomem *addr;
 	u32 status;
 
@@ -698,10 +697,17 @@  static void __cxl_handle_cor_ras(struct device *dev,
 
 	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
 	status = readl(addr);
-	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
-		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
+
+	if (!(status & CXL_RAS_CORRECTABLE_STATUS_MASK))
+		return;
+
+	writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
+	if (is_cxl_memdev(dev)) {
+		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+
 		trace_cxl_aer_correctable_error(cxlmd, status);
-	}
+	} else if (dev_is_pci(dev))
+		trace_cxl_port_aer_correctable_error(dev, status);
 }
 
 static void cxl_handle_endpoint_cor_ras(struct cxl_dev_state *cxlds)
@@ -733,7 +739,6 @@  static void header_log_copy(void __iomem *ras_base, u32 *log)
 static bool __cxl_handle_ras(struct device *dev,
 			     void __iomem *ras_base)
 {
-	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	u32 hl[CXL_HEADERLOG_SIZE_U32];
 	void __iomem *addr;
 	u32 status;
@@ -759,7 +764,13 @@  static bool __cxl_handle_ras(struct device *dev,
 	}
 
 	header_log_copy(ras_base, hl);
-	trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
+	if (is_cxl_memdev(dev)) {
+		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+
+		trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
+	} else if (dev_is_pci(dev))
+		trace_cxl_port_aer_uncorrectable_error(dev, status);
+
 	writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
 
 	return true;
@@ -882,6 +893,80 @@  static bool cxl_handle_rdport_ras(struct cxl_dev_state *cxlds,
 	return __cxl_handle_ras(&cxlds->cxlmd->dev, dport->regs.ras);
 }
 
+static int match_uport(struct device *dev, void *data)
+{
+	struct device *uport_dev = (struct device *)data;
+	struct cxl_port *port;
+
+	if (!is_cxl_port(dev))
+		return 0;
+
+	port = to_cxl_port(dev);
+
+	return (port->uport_dev == uport_dev);
+}
+
+static struct cxl_port *pci_to_cxl_uport(struct pci_dev *pdev)
+{
+	struct cxl_dport *dport;
+	struct device *port_dev;
+	struct cxl_port *port;
+
+	port = find_cxl_port(pdev->dev.parent, &dport);
+	if (!port)
+		return NULL;
+	put_device(&port->dev);
+
+	port_dev = device_find_child(&port->dev, &pdev->dev, match_uport);
+	if (!port_dev)
+		return NULL;
+	put_device(port_dev);
+
+	port = to_cxl_port(port_dev);
+
+	return port;
+}
+
+static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
+{
+	void __iomem *ras_base = NULL;
+
+	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
+	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
+		struct cxl_dport *dport;
+
+		find_cxl_port(&pdev->dev, &dport);
+		ras_base = dport ? dport->regs.ras : NULL;
+	} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM) {
+		struct cxl_port *port = pci_to_cxl_uport(pdev);
+
+		ras_base = port ? port->regs.ras : NULL;
+	}
+
+	return ras_base;
+}
+
+int port_internal_err_cb(struct notifier_block *unused,
+			 unsigned long event, void *ptr)
+{
+	struct pci_dev *pdev = (struct pci_dev *)ptr;
+	void __iomem *ras_base;
+
+	if (!pdev)
+		return 0;
+
+	if (event == AER_CORRECTABLE) {
+		ras_base = cxl_pci_port_ras(pdev);
+		__cxl_handle_cor_ras(&pdev->dev, ras_base);
+	} else if ((event == AER_FATAL) || (event == AER_NONFATAL)) {
+		ras_base = cxl_pci_port_ras(pdev);
+		__cxl_handle_ras(&pdev->dev, ras_base);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(port_internal_err_cb, CXL);
+
 /*
  * Copy the AER capability registers using 32 bit read accesses.
  * This is necessary because RCRB AER capability is MMIO mapped. Clear the
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 887ed6e358fb..d0f95c965ab4 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1027,7 +1027,7 @@  void put_cxl_root(struct cxl_root *cxl_root)
 }
 EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL);
 
-static struct cxl_dport *find_dport(struct cxl_port *port, int id)
+struct cxl_dport *find_dport(struct cxl_port *port, int id)
 {
 	struct cxl_dport *dport;
 	unsigned long index;
@@ -1336,8 +1336,8 @@  static struct cxl_port *__find_cxl_port(struct cxl_find_port_ctx *ctx)
 	return NULL;
 }
 
-static struct cxl_port *find_cxl_port(struct device *dport_dev,
-				      struct cxl_dport **dport)
+struct cxl_port *find_cxl_port(struct device *dport_dev,
+			       struct cxl_dport **dport)
 {
 	struct cxl_find_port_ctx ctx = {
 		.dport_dev = dport_dev,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 7cee678fdb75..04725344393b 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -11,6 +11,7 @@ 
 #include <linux/log2.h>
 #include <linux/node.h>
 #include <linux/io.h>
+#include "../pci/pcie/portdrv.h"
 
 /**
  * DOC: cxl objects
@@ -760,11 +761,15 @@  struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
 #ifdef CONFIG_PCIEAER_CXL
 void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
 void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
+int port_internal_err_cb(struct notifier_block *unused,
+			 unsigned long event, void *ptr);
 #else
 static inline void cxl_setup_parent_dport(struct device *host,
 					  struct cxl_dport *dport) { }
 static inline void cxl_setup_parent_uport(struct device *host,
 					  struct cxl_port *port) { }
+static inline int port_internal_err_cb(struct notifier_block *unused,
+				unsigned long event, void *ptr) { return 0; }
 #endif
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 93992a1c8eec..6044955e1c48 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -130,4 +130,6 @@  void read_cdat_data(struct cxl_port *port);
 void cxl_cor_error_detected(struct pci_dev *pdev);
 pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
 				    pci_channel_state_t state);
+int port_err_nb_cb(struct notifier_block *unused,
+		   unsigned long event, void *ptr);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..f4183c5aea38 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -926,6 +926,10 @@  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return rc;
 }
 
+struct notifier_block port_internal_err_nb = {
+	.notifier_call = port_internal_err_cb,
+};
+
 static const struct pci_device_id cxl_mem_pci_tbl[] = {
 	/* PCI class code for CXL.mem Type-3 Devices */
 	{ PCI_DEVICE_CLASS((PCI_CLASS_MEMORY_CXL << 8 | CXL_MEMORY_PROGIF), ~0)},
@@ -974,6 +978,19 @@  static struct pci_driver cxl_pci_driver = {
 	},
 };
 
-module_pci_driver(cxl_pci_driver);
+static int __init cxl_pci_init(void)
+{
+	atomic_notifier_chain_register(&portdrv_aer_internal_err_chain, &port_internal_err_nb);
+	return pci_register_driver(&cxl_pci_driver);
+}
+module_init(cxl_pci_init);
+
+static void __exit cxl_pci_exit(void)
+{
+	atomic_notifier_chain_unregister(&portdrv_aer_internal_err_chain, &port_internal_err_nb);
+	pci_unregister_driver(&cxl_pci_driver);
+}
+module_exit(cxl_pci_exit);
+
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(CXL);