Date:   Thu, 13 Apr 2023 14:13:16 -0500
From:   Terry Bowman <Terry.Bowman@....com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     alison.schofield@...el.com, vishal.l.verma@...el.com,
        ira.weiny@...el.com, bwidawsk@...nel.org, dan.j.williams@...el.com,
        dave.jiang@...el.com, linux-cxl@...r.kernel.org, rrichter@....com,
        linux-kernel@...r.kernel.org, bhelgaas@...gle.com
Subject: Re: [PATCH v3 1/6] cxl/pci: Add RCH downstream port AER and RAS
 register discovery

Hi Jonathan,

Thanks for the review. I added comments below.

On 4/13/23 10:30, Jonathan Cameron wrote:
> On Tue, 11 Apr 2023 13:02:57 -0500
> Terry Bowman <terry.bowman@....com> wrote:
> 
>> Restricted CXL host (RCH) downstream port AER information is not currently
>> logged while in the error state. One problem preventing existing PCIe AER
>> functions from logging errors is the AER registers are not accessible. The
>> CXL driver requires changes to find RCH downstream port AER registers for
>> purpose of error logging.
>>
>> RCH downstream ports are not enumerated during a PCI bus scan and are
>> instead discovered using system firmware, ACPI in this case.[1] The
>> downstream port is implemented as a Root Complex Register Block (RCRB).
>> The RCRB is a 4k memory block containing PCIe registers based on the PCIe
>> root port.[2] The RCRB includes AER extended capability registers used for
>> reporting errors. Note, the RCH's AER Capability is located in the RCRB
>> memory space instead of PCI configuration space, thus its register access
>> is different. Existing kernel PCIe AER functions can not be used to manage
>> the downstream port AER capabilities because the port was not enumerated
>> during PCI scan and the registers are not PCI config accessible.
>>
>> Discover RCH downstream port AER extended capability registers. This
>> requires using MMIO accesses to search for extended AER capability in
>> RCRB register space.
>>
>> [1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
>> [2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
>>
>> Co-developed-by: Robert Richter <rrichter@....com>
>> Signed-off-by: Robert Richter <rrichter@....com>
>> Signed-off-by: Terry Bowman <terry.bowman@....com>
> 
> Hi Terry,
> 
> Sorry I missed first few versions.  Playing catch up.
> 
> A few minor comments only inline.
> 
> 
> 
>> ---
>>  drivers/cxl/core/regs.c | 93 +++++++++++++++++++++++++++++++++++------
>>  drivers/cxl/cxl.h       |  5 +++
>>  drivers/cxl/mem.c       | 39 +++++++++++------
>>  3 files changed, 113 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index 1476a0299c9b..bde1fffab09e 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -332,10 +332,36 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>>  
>> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
>> +				 char *name)
> 
> dev isn't used.
> 

'dev' was used earlier for logging that is since removed.

>> +{
>> +
> 
> Trivial but no point in blank line here.
> 

I'll remove it.

>> +	if (!request_mem_region(map->resource, map->max_size, name))
>> +		return NULL;
>> +
>> +	map->base = ioremap(map->resource, map->max_size);
>> +	if (!map->base) {
>> +		release_mem_region(map->resource, map->max_size);
>> +		return NULL;
>> +	}
>> +
>> +	return map->base;
> 
> Why return a value you've already stashed in map->base?
> 
This allowed for a clean return check where cxl_map_reg() is called.
This could/should have been a boolean. This will be fixed with the refactoring 
mentioned below.

>> +}
>> +
> 
> This is similar enough to devm_cxl_iomap_block() that I'd kind
> of like them them take the same parameters.  That would mean
> moving the map structure outside of the calls and instead passing
> in the 3 relevant parameters.  Perhaps not worth it.
> 
The intent was to cleanup the cxl_map_reg() callers.  Using a 'struct 
cxl_register_map' carries all the variables required for mapping and reduces 
the number of variables otherwise declared in the callers. But, I understand 
why a common interface is preferred in this case.

Ok. I'll change the parameters and return value to match devm_cxl_iomap_block(). 

>> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
>> +{
> 
> dev isn't used here either. Makes little sense to pass it in to either funtion.
> 
>> +	iounmap(map->base);
>> +	release_mem_region(map->resource, map->max_size);
>> +}
>> +
>>  resource_size_t cxl_rcrb_to_component(struct device *dev,
>>  				      resource_size_t rcrb,
>>  				      enum cxl_rcrb which)
>>  {
>> +	struct cxl_register_map map = {
>> +		.resource = rcrb,
>> +		.max_size = SZ_4K
>> +	};
>>  	resource_size_t component_reg_phys;
>>  	void __iomem *addr;
>>  	u32 bar0, bar1;
>> @@ -343,7 +369,10 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>>  	u32 id;
>>  
>>  	if (which == CXL_RCRB_UPSTREAM)
>> -		rcrb += SZ_4K;
>> +		map.resource += SZ_4K;
>> +
>> +	if (!cxl_map_reg(dev, &map, "CXL RCRB"))
>> +		return CXL_RESOURCE_NONE;
>>  
>>  	/*
>>  	 * RCRB's BAR[0..1] point to component block containing CXL
>> @@ -351,21 +380,12 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>>  	 * the PCI Base spec here, esp. 64 bit extraction and memory
>>  	 * ranges alignment (6.0, 7.5.1.2.1).
>>  	 */
>> -	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
>> -		return CXL_RESOURCE_NONE;
>> -	addr = ioremap(rcrb, SZ_4K);
>> -	if (!addr) {
>> -		dev_err(dev, "Failed to map region %pr\n", addr);
>> -		release_mem_region(rcrb, SZ_4K);
>> -		return CXL_RESOURCE_NONE;
>> -	}
>> -
>> +	addr = map.base;
> 
> I'd have preferred to see this refactor as a precursor patch to the
> 'real changes' that follow.
> 

Ok. I can make the cxl_map_reg() addition and cxl_rcrb_to_component() refactor 
to a separate patch.

>>  	id = readl(addr + PCI_VENDOR_ID);
>>  	cmd = readw(addr + PCI_COMMAND);
>>  	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
>>  	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
>> -	iounmap(addr);
>> -	release_mem_region(rcrb, SZ_4K);
>> +	cxl_unmap_reg(dev, &map);
>>  
>>  	/*
>>  	 * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
>> @@ -396,3 +416,52 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>>  	return component_reg_phys;
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> 
> 
> ...
> 
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 044a92d9813e..df64c402e6e6 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -270,6 +270,9 @@ enum cxl_rcrb {
>>  resource_size_t cxl_rcrb_to_component(struct device *dev,
>>  				      resource_size_t rcrb,
>>  				      enum cxl_rcrb which);
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
>> +u16 cxl_component_to_ras(struct device *dev,
>> +			 resource_size_t component_reg_phys);
>>  
>>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>>  #define CXL_TARGET_STRLEN 20
>> @@ -601,6 +604,8 @@ struct cxl_dport {
>>  	int port_id;
>>  	resource_size_t component_reg_phys;
>>  	resource_size_t rcrb;
>> +	u16 aer_cap;
>> +	u16 ras_cap;
> 
> This structure has kernel-doc that needs to be updated for these new entries.
> 

I'll add.

>>  	bool rch;
>>  	struct cxl_port *port;
>>  };
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 39c4b54f0715..014295ab6bc6 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -45,13 +45,36 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>>  	return 0;
>>  }
>>  
>> +static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
>> +			   struct cxl_dport *parent_dport)
>> +{
>> +	struct cxl_memdev *cxlmd  = cxlds->cxlmd;
> 
> extra space before =
>

Ok. Ill remove the extra space.
 
>> +
>> +	if (!parent_dport->rch)
>> +		return;
>> +
>> +	/*
>> +	 * The component registers for an RCD might come from the
>> +	 * host-bridge RCRB if they are not already mapped via the
>> +	 * typical register locator mechanism.
>> +	 */
>> +	if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
>> +		cxlds->component_reg_phys = cxl_rcrb_to_component(
>> +			&cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
>> +
>> +	parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
>> +						parent_dport->rcrb);
>> +
>> +	parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
>> +						     parent_dport->component_reg_phys);
>> +}
>> +
>>  static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>>  				 struct cxl_dport *parent_dport)
>>  {
>>  	struct cxl_port *parent_port = parent_dport->port;
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  	struct cxl_port *endpoint, *iter, *down;
>> -	resource_size_t component_reg_phys;
>>  	int rc;
>>  
>>  	/*
>> @@ -66,17 +89,9 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>>  		ep->next = down;
>>  	}
>>  
>> -	/*
>> -	 * The component registers for an RCD might come from the
>> -	 * host-bridge RCRB if they are not already mapped via the
>> -	 * typical register locator mechanism.
>> -	 */
>> -	if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
>> -		component_reg_phys = cxl_rcrb_to_component(
>> -			&cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
>> -	else
>> -		component_reg_phys = cxlds->component_reg_phys;
>> -	endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
>> +	cxl_setup_rcrb(cxlds, parent_dport);
>> +
>> +	endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys,
>>  				     parent_dport);
> As above, I'd prefer to see this refactor done in a precursor patch before the new
> stuff is added.  I like reviewing noop patches as I don't have to think much (so
> can do it when I'm supposedly in a meeting ;)
> 
 
Ok. I'll add an earlier patch that introduces cxl_setup_rcrb() and first moves this 
chunk into cxl_setup_rcrb(). The following patch will replace the cxl_setup_rcrb() 
logic with the AER and RAS discovery.

My understanding is the requested refactoring changes then splits this patch into 
the 3 patches listed below (using git log latest first order): 
- Add RCH downstream port AER and RAS register discovery
- Refactor RCD component discovery into separate function
- Refactor RCRB register mapping into separate function

Regards,
Terry

> Jonathan
>>  	if (IS_ERR(endpoint))
>>  		return PTR_ERR(endpoint);
> 

Powered by blists - more mailing lists