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

Dan Williams
2023-06-12T20:29:23Z

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@amd.com>
> ---
> 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?

> +
> + /*
> + * 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?

> +}
> +
> 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.