RE: [PATCH v10 10/15] cxl/pci: Map RCH downstream AER registers for logging protocol errors

[Dan Williams]

Terry Bowman wrote:
> The restricted CXL host (RCH) error handler will log protocol errors
> using AER and RAS status registers. The AER and RAS registers need
> to be virtually memory mapped before enabling interrupts. Update
> __devm_cxl_add_dport() to include RCH RAS and AER mapping.
> 
> Add 'struct cxl_regs' to 'struct cxl_dport' for saving a pointer to
> the RCH downstream port's AER and RAS registers.
> 
> Co-developed-by: Robert Richter <rrichter@xxxxxxx>
> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> ---
>  drivers/cxl/core/port.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/regs.c |  1 +
>  drivers/cxl/cxl.h       | 12 ++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 45f8846d8c8a..2a22a7ed4704 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -8,6 +8,7 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> +#include <linux/aer.h>
>  #include <cxlmem.h>
>  #include <cxlpci.h>
>  #include <cxl.h>
> @@ -949,6 +950,37 @@ static void cxl_dport_unlink(void *data)
>  	sysfs_remove_link(&port->dev.kobj, link_name);
>  }
>  
> +static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
> +{
> +	struct cxl_rcrb_info *ri = &dport->rcrb;
> +	struct cxl_port *port = dport->port;
> +	void __iomem *dport_aer = NULL;
> +	resource_size_t aer_phys;
> +
> +	if (dport->rch && ri->aer_cap) {
> +		aer_phys = ri->aer_cap + ri->base;
> +		dport_aer = devm_cxl_iomap_block(&port->dev, aer_phys,
> +				sizeof(struct aer_capability_regs));
> +	}
> +
> +	dport->regs.dport_aer = dport_aer;
> +}
> +
> +static void cxl_dport_map_regs(struct cxl_dport *dport)
> +{
> +	struct cxl_register_map *map = &dport->comp_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, dev, &dport->regs.component,
> +					BIT(CXL_CM_CAP_CAP_ID_RAS)))
> +		dev_dbg(dev, "Failed to map RAS capability.\n");
> +
> +	if (dport->rch)
> +		cxl_dport_map_rch_aer(dport);
> +}
> +
>  static struct cxl_dport *
>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  		     int port_id, resource_size_t component_reg_phys,
> @@ -1008,6 +1040,8 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	if (rc)
>  		return ERR_PTR(rc);
>  
> +	cxl_dport_map_regs(dport);
> +

Mapping registers for usage is a driver operation, not an enumeration
operation, so this should move out of add_dport, and it should fail the
driver load if it fails.

Yes this happens to be the case that dports are only enumerated in
cxl_port driver probe path, but that need not be the case forever and no
other parts of the CXL core are lighting up registers from an
enumeration routine.