On 12/12/2024 4:36 AM, Alejandro Lucero Palau wrote:
> On 12/11/24 23:39, Terry Bowman wrote:
>> The CXL mem driver (cxl_mem) currently maps and caches a pointer to RAS
>> registers for the endpoint's Root Port. The same needs to be done for
>> each of the CXL Downstream Switch Ports and CXL Root Ports found between
>> the endpoint and CXL Host Bridge.
>>
>> Introduce cxl_init_ep_ports_aer() to be called for each CXL Port in the
>> sub-topology between the endpoint and the CXL Host Bridge. This function
>> will determine if there are CXL Downstream Switch Ports or CXL Root Ports
>> associated with this Port. The same check will be added in the future for
>> upstream switch ports.
>>
>> Move the RAS register map logic from cxl_dport_map_ras() into
>> cxl_dport_init_ras_reporting(). This eliminates the need for the helper
>> function, cxl_dport_map_ras().
>>
>> cxl_init_ep_ports_aer() calls cxl_dport_init_ras_reporting() to map
>> the RAS registers for CXL Downstream Switch Ports and CXL Root Ports.
>>
>> cxl_dport_init_ras_reporting() must check for previously mapped registers
>> before mapping. This is necessary because endpoints under a CXL switch
>> may share CXL Downstream Switch Ports or CXL Root Ports. Ensure the port
>> registers are only mapped once.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
>
> Reviewed-by: Alejandro Lucero <alucerop@xxxxxxx>
>
>
> Just a nit but, regs mapping could fail, what is properly reported, and
> the __cxl_handle_ras function checks for the regs iomem being there
> before using them. But if the mapping failed, any report there is
> silently dropped. If the AER is happening, maybe to add a WARN_ONCE there?
>
Good point. I'll add the WARN_ONCE().
- Terry
>> ---
>> drivers/cxl/core/pci.c | 37 +++++++++++++++----------------------
>> drivers/cxl/cxl.h | 6 ++----
>> drivers/cxl/mem.c | 31 +++++++++++++++++++++++++++++--
>> 3 files changed, 46 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 5b46bc46aaa9..8540d1fd2e25 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -749,18 +749,6 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
>> }
>> }
>>
>> -static void cxl_dport_map_ras(struct cxl_dport *dport)
>> -{
>> - struct cxl_register_map *map = &dport->reg_map;
>> - struct device *dev = dport->dport_dev;
>> -
>> - if (!map->component_map.ras.valid)
>> - dev_dbg(dev, "RAS registers not found\n");
>> - else if (cxl_map_component_regs(map, &dport->regs.component,
>> - BIT(CXL_CM_CAP_CAP_ID_RAS)))
>> - dev_dbg(dev, "Failed to map RAS capability.\n");
>> -}
>> -
>> static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>> {
>> void __iomem *aer_base = dport->regs.dport_aer;
>> @@ -788,22 +776,27 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>> /**
>> * cxl_dport_init_ras_reporting - Setup CXL RAS report on this dport
>> * @dport: the cxl_dport that needs to be initialized
>> - * @host: host device for devm operations
>> */
>> -void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
>> +void cxl_dport_init_ras_reporting(struct cxl_dport *dport)
>> {
>> - dport->reg_map.host = host;
>> - cxl_dport_map_ras(dport);
>> -
>> - if (dport->rch) {
>> - struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport->dport_dev);
>> -
>> - if (!host_bridge->native_aer)
>> - return;
>> + struct device *dport_dev = dport->dport_dev;
>> + struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport_dev);
>>
>> + dport->reg_map.host = dport_dev;
>> + if (dport->rch && host_bridge->native_aer) {
>> cxl_dport_map_rch_aer(dport);
>> cxl_disable_rch_root_ints(dport);
>> }
>> +
>> + /* dport may have more than 1 downstream EP. Check if already mapped. */
>> + if (dport->regs.ras)
>> + return;
>> +
>> + if (cxl_map_component_regs(&dport->reg_map, &dport->regs.component,
>> + BIT(CXL_CM_CAP_CAP_ID_RAS))) {
>> + dev_err(dport_dev, "Failed to map RAS capability.\n");
>> + return;
>> + }
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_dport_init_ras_reporting, CXL);
>>
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 5406e3ab3d4a..51acca3415b4 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -763,11 +763,9 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>> resource_size_t rcrb);
>>
>> #ifdef CONFIG_PCIEAER_CXL
>> -void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
>> -void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host);
>> +void cxl_dport_init_ras_reporting(struct cxl_dport *dport);
>> #else
>> -static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport,
>> - struct device *host) { }
>> +static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport) { }
>> #endif
>>
>> struct cxl_decoder *to_cxl_decoder(struct device *dev);
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index a9fd5cd5a0d2..0ae89c9da71e 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -45,6 +45,31 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>> return 0;
>> }
>>
>> +static bool dev_is_cxl_pci(struct device *dev, u32 pcie_type)
>> +{
>> + struct pci_dev *pdev;
>> +
>> + if (!dev || !dev_is_pci(dev))
>> + return false;
>> +
>> + pdev = to_pci_dev(dev);
>> +
>> + return (pci_pcie_type(pdev) == pcie_type);
>> +}
>> +
>> +static void cxl_init_ep_ports_aer(struct cxl_ep *ep)
>> +{
>> + struct cxl_dport *dport = ep->dport;
>> +
>> + if (dport) {
>> + struct device *dport_dev = dport->dport_dev;
>> +
>> + if (dev_is_cxl_pci(dport_dev, PCI_EXP_TYPE_DOWNSTREAM) ||
>> + dev_is_cxl_pci(dport_dev, PCI_EXP_TYPE_ROOT_PORT))
>> + cxl_dport_init_ras_reporting(dport);
>> + }
>> +}
>> +
>> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>> struct cxl_dport *parent_dport)
>> {
>> @@ -52,6 +77,9 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>> struct cxl_port *endpoint, *iter, *down;
>> int rc;
>>
>> + if (parent_dport->rch)
>> + cxl_dport_init_ras_reporting(parent_dport);
>> +
>> /*
>> * Now that the path to the root is established record all the
>> * intervening ports in the chain.
>> @@ -62,6 +90,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>>
>> ep = cxl_ep_load(iter, cxlmd);
>> ep->next = down;
>> + cxl_init_ep_ports_aer(ep);
>> }
>>
>> /* Note: endpoint port component registers are derived from @cxlds */
>> @@ -166,8 +195,6 @@ static int cxl_mem_probe(struct device *dev)
>> else
>> endpoint_parent = &parent_port->dev;
>>
>> - cxl_dport_init_ras_reporting(dport, dev);
>> -
>> scoped_guard(device, endpoint_parent) {
>> if (!endpoint_parent->driver) {
>> dev_err(dev, "CXL port topology %s not enabled\n",
