Re: [PATCH v10 09/17] cxl/pci: Map CXL Endpoint Port and CXL Switch Port RAS registers

[Dave Jiang <dave.jiang@intel.com>]

On 7/18/25 3:40 PM, Bowman, Terry wrote:
> On 7/18/2025 5:01 PM, Dave Jiang wrote:
>>
>>
>> On 7/18/25 2:55 PM, Bowman, Terry wrote:
>>>
>>>
>>> On 7/18/2025 4:28 PM, Dave Jiang wrote:
>>>>
>>>> On 6/26/25 3:42 PM, Terry Bowman wrote:
>>>>> CXL Endpoint (EP) Ports may include Root Ports (RP) or Downstream Switch
>>>>> Ports (DSP). CXL RPs and DSPs contain RAS registers that require memory
>>>>> mapping to enable RAS logging. This initialization is currently missing and
>>>>> must be added for CXL RPs and DSPs.
>>>>>
>>>>> Update cxl_dport_init_ras_reporting() to support RP and DSP RAS mapping.
>>>>> Add alongside the existing Restricted CXL Host Downstream Port RAS mapping.
>>>>>
>>>>> Update cxl_endpoint_port_probe() to invoke cxl_dport_init_ras_reporting().
>>>>> This will initiate the RAS mapping for CXL RPs and DSPs when each CXL EP is
>>>>> created and added to the EP port.
>>>>>
>>>>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>>>>> ---
>>>>>  drivers/cxl/cxl.h  |  2 ++
>>>>>  drivers/cxl/mem.c  |  3 ++-
>>>>>  drivers/cxl/port.c | 58 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>  3 files changed, 61 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>>>>> index c57c160f3e5e..d696d419bd5a 100644
>>>>> --- a/drivers/cxl/cxl.h
>>>>> +++ b/drivers/cxl/cxl.h
>>>>> @@ -590,6 +590,7 @@ struct cxl_dax_region {
>>>>>   * @parent_dport: dport that points to this port in the parent
>>>>>   * @decoder_ida: allocator for decoder ids
>>>>>   * @reg_map: component and ras register mapping parameters
>>>>> + * @uport_regs: mapped component registers
>>>>>   * @nr_dports: number of entries in @dports
>>>>>   * @hdm_end: track last allocated HDM decoder instance for allocation ordering
>>>>>   * @commit_end: cursor to track highest committed decoder for commit ordering
>>>>> @@ -610,6 +611,7 @@ struct cxl_port {
>>>>>  	struct cxl_dport *parent_dport;
>>>>>  	struct ida decoder_ida;
>>>>>  	struct cxl_register_map reg_map;
>>>>> +	struct cxl_component_regs uport_regs;
>>>>>  	int nr_dports;
>>>>>  	int hdm_end;
>>>>>  	int commit_end;
>>>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>>>>> index 6e6777b7bafb..d2155f45240d 100644
>>>>> --- a/drivers/cxl/mem.c
>>>>> +++ b/drivers/cxl/mem.c
>>>>> @@ -166,7 +166,8 @@ static int cxl_mem_probe(struct device *dev)
>>>>>  	else
>>>>>  		endpoint_parent = &parent_port->dev;
>>>>>  
>>>>> -	cxl_dport_init_ras_reporting(dport, dev);
>>>>> +	if (dport->rch)
>>>>> +		cxl_dport_init_ras_reporting(dport, dev);
>>>>>  
>>>>>  	scoped_guard(device, endpoint_parent) {
>>>>>  		if (!endpoint_parent->driver) {
>>>>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>>>>> index 021f35145c65..b52f82925891 100644
>>>>> --- a/drivers/cxl/port.c
>>>>> +++ b/drivers/cxl/port.c
>>>>> @@ -111,6 +111,17 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>>>>>  	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
>>>>>  }
>>>>>  
>>>>> +static void cxl_uport_init_ras_reporting(struct cxl_port *port,
>>>>> +					 struct device *host)
>>>>> +{
>>>>> +	struct cxl_register_map *map = &port->reg_map;
>>>>> +
>>>>> +	map->host = host;
>>>>> +	if (cxl_map_component_regs(map, &port->uport_regs,
>>>>> +				   BIT(CXL_CM_CAP_CAP_ID_RAS)))
>>>>> +		dev_dbg(&port->dev, "Failed to map RAS capability\n");
>>>>> +}
>>>>> +
>>>>>  /**
>>>>>   * cxl_dport_init_ras_reporting - Setup CXL RAS report on this dport
>>>>>   * @dport: the cxl_dport that needs to be initialized
>>>>> @@ -119,7 +130,6 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>>>>>  void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
>>>>>  {
>>>>>  	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);
>>>>> @@ -127,12 +137,54 @@ void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
>>>>>  		if (!host_bridge->native_aer)
>>>>>  			return;
>>>>>  
>>>>> +		cxl_dport_map_ras(dport);
>>>>>  		cxl_dport_map_rch_aer(dport);
>>>>>  		cxl_disable_rch_root_ints(dport);
>>>>> +		return;
>>>>>  	}
>>>>> +
>>>>> +	if (cxl_map_component_regs(&dport->reg_map, &dport->regs.component,
>>>>> +				   BIT(CXL_CM_CAP_CAP_ID_RAS)))
>>>>> +		dev_dbg(dport->dport_dev, "Failed to map RAS capability\n");
>>>>> +
>>>>>  }
>>>>>  EXPORT_SYMBOL_NS_GPL(cxl_dport_init_ras_reporting, "CXL");
>>>>>  
>>>>> +static void cxl_switch_port_init_ras(struct cxl_port *port)
>>>>> +{
>>>>> +	if (is_cxl_root(to_cxl_port(port->dev.parent)))
>>>>> +		return;
>>>>> +
>>>>> +	/* May have upstream DSP or RP */
>>>>> +	if (port->parent_dport && dev_is_pci(port->parent_dport->dport_dev)) {
>>>>> +		struct pci_dev *pdev = to_pci_dev(port->parent_dport->dport_dev);
>>>>> +
>>>>> +		if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
>>>>> +		    (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM))
>>>>> +			cxl_dport_init_ras_reporting(port->parent_dport, &port->dev);
>>>>> +	}
>>>>> +
>>>>> +	cxl_uport_init_ras_reporting(port, &port->dev);
>>>>> +}
>>>>> +
>>>>> +static void cxl_endpoint_port_init_ras(struct cxl_port *port)
>>>> Maybe rename 'port' to 'ep' to be explicit
>>> Ok
>>>>> +{
>>>>> +	struct cxl_dport *dport;
>>>> parent_dport would be clearer. I was thinking why does the endpoint have a dport for a second there.
>>> Ok
>>>>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>>>>> +	struct cxl_port *parent_port __free(put_cxl_port) =
>>>>> +		cxl_mem_find_port(cxlmd, &dport);
>>>>> +
>>>>> +	if (!dport || !dev_is_pci(dport->dport_dev)) {
>>>>> +		dev_err(&port->dev, "CXL port topology not found\n");> +		return;
>>>>> +	}
>>>>> +
>>>>> +	cxl_dport_init_ras_reporting(dport, cxlmd->cxlds->dev);
>>>>> +}
>>>>> +
>>>>> +#else
>>>>> +static void cxl_endpoint_port_init_ras(struct cxl_port *port) { }
>>>>> +static void cxl_switch_port_init_ras(struct cxl_port *port) { }
>>>>>  #endif /* CONFIG_PCIEAER_CXL */
>>>> I cc'd you on the new patch to move all the AER stuff to core/pci_aer.c. That should take care of ifdef CONFIG_PCIEAER_CXL in pci.c and port.c.
>>>>
>>>> DJ
>>>
>>> Move to core/native_ras.c introduced in "Dequeue forwarded CXL error", right? I just want to be certain.
>>
>> I just posted this [1] to clean up what's there. Do you prefer me to just rename the file to native_ras.c? Or maybe pci_ras.c?
>>
>> [1]: https://lore.kernel.org/linux-cxl/20250718212452.2100663-1-dave.jiang@intel.com/
>>
>> DJ
>>
> 
> Hi Dave,
> 
> I think leaving as you have it is fine. 
> 
> Would you like to see my v10 changes in core/native_ras.c stay as-is or move into pci_aer.c?

Yes. Let's move it all to pci_aer.c in v11 if no one objects to my changes. Thanks!

DJ

> 
> -Terry
> 
>>>
>>> Regards,
>>> Terry
>>>
>>>>>  >  static int cxl_switch_port_probe(struct cxl_port *port)
>>>>> @@ -149,6 +201,8 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>>>>  
>>>>>  	cxl_switch_parse_cdat(port);
>>>>>  
>>>>> +	cxl_switch_port_init_ras(port);
>>>>> +
>>>>>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>>>>>  	if (!IS_ERR(cxlhdm))
>>>>>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>>>>> @@ -203,6 +257,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>>>>  	if (rc)
>>>>>  		return rc;
>>>>>  
>>>>> +	cxl_endpoint_port_init_ras(port);
>>>>> +
>>>>>  	/*
>>>>>  	 * Now that all endpoint decoders are successfully enumerated, try to
>>>>>  	 * assemble regions from committed decoders
>>>
>>
> 
>