Re: [PATCH v5 23/26] cxl/pci: Disable root port interrupts in RCH mode

[Terry Bowman]

Hi Dan,

Thanks for the review.

On 6/12/23 15:29, Dan Williams wrote:
> Terry Bowman wrote:
>> The RCH root port contains root command AER registers that should not be
>> enabled.[1] Disable these to prevent root port interrupts.
>>
>> [1] CXL 3.0 - 12.2.1.1 RCH Downstream Port-detected Errors
>>
>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
>> ---
>>  drivers/cxl/core/port.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index bc5d0ee9da54..828ae69086c4 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -981,6 +981,30 @@ static int cxl_dport_map_regs(struct cxl_dport *dport)
>>  	return cxl_dport_map_rch_aer(dport);
>>  }
>>  
>> +static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>> +{
>> +	void __iomem *aer_base = dport->regs.dport_aer;
>> +	u32 aer_cmd_mask, aer_cmd;
>> +
>> +	if (!dport->rch || !aer_base)
>> +		return;
> 
> Does this need to check ->rch? There is no path that sets
> ->regs.dport_aer in the VH case, right?
> 

Correct. This can be simplified to only check !aer_base because 
aer_base is only set if dport->rch.

>> +
>> +	/*
>> +	 * Disable RCH root port command interrupts.
>> +	 * CXL 3.0 12.2.1.1 - RCH Downstream Port-detected Errors
>> +	 *
>> +	 * This sequnce may not be necessary. CXL spec states disabling
>> +	 * the root cmd register's interrupts is required. But, PCI spec
>> +	 * shows these are disabled by default on reset.
>> +	 */
>> +	aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
>> +			PCI_ERR_ROOT_CMD_NONFATAL_EN |
>> +			PCI_ERR_ROOT_CMD_FATAL_EN);
>> +	aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
>> +	aer_cmd &= ~aer_cmd_mask;
>> +	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
> 
> What about the scenario where @host_bridge->native_cxl_error is false?
> Should the driver unconditionally touch AER registers?
> 

Right. The driver should only touch the AER registers if the AER control is OSC negotiated to OS.
I'll add check for @host_bridge->native_cxl_error. 

>> +}
>> +
>>  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,
>> @@ -1038,6 +1062,8 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	if (rc && rc != -ENODEV)
>>  		return ERR_PTR(rc);
>>  
>> +	cxl_disable_rch_root_ints(dport);
>> +
> 
> It feels odd to modify hardware state within an object setup helper. Is
> there a reason this needs to be settled before __devm_cxl_add_dport()
> returns? If not this feels like a helper that cxl_acpi should call to
> change the state of the @dport instance it allocated.
Sure. This can be moved into the caller, add_host_bridge_dport(), after calling devm_cxl_add_rch_dport().

Regards,
Terry