Message-ID: <6b4b451c-935b-4ce6-ab64-c2e66311099a@amd.com>
Date: Mon, 24 Jun 2024 11:09:41 -0500
From: Terry Bowman <Terry.Bowman@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: dan.j.williams@...el.com, ira.weiny@...el.com, dave@...olabs.net,
 dave.jiang@...el.com, alison.schofield@...el.com, ming4.li@...el.com,
 vishal.l.verma@...el.com, jim.harris@...sung.com,
 ilpo.jarvinen@...ux.intel.com, ardb@...nel.org,
 sathyanarayanan.kuppuswamy@...ux.intel.com, linux-cxl@...r.kernel.org,
 linux-kernel@...r.kernel.org, Yazen.Ghannam@....com, Robert.Richter@....com
Subject: Re: [RFC PATCH 7/9] cxl/pci: Add atomic notifier callback for CXL
 PCIe port AER internal errors

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@....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@....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);
> 

Powered by blists - more mailing lists