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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64878016974b0_1433ac29473@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Mon, 12 Jun 2023 13:29:10 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Terry Bowman <terry.bowman@....com>, <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>, <Jonathan.Cameron@...wei.com>,
        <linux-cxl@...r.kernel.org>
CC:     <terry.bowman@....com>, <rrichter@....com>,
        <linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>
Subject: RE: [PATCH v5 23/26] cxl/pci: Disable root port interrupts in RCH
 mode

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@....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.