RE: [PATCH v7 4/5] cxl/port, EINJ: Add CXL EINJ callback functions

[Dan Williams <dan.j.williams@intel.com>]

Ben Cheatham wrote:
> Add functions to the EINJ module to be used in the CXL module for CXL
> protocol error injection. The callbacks implement the einj_types and
> einj_inject files under /sys/kernel/debug/cxl/portX/dportY. These two
> files work in the same way as the available_error_types and error_inject
> files under /sys/kernel/debug/apei/einj, but only for CXL error types.
> If the dport is enumerated in RCH (CXL 1.1) mode, a CXL 1.1 error is
> injected into the dport; a CXL 2.0 error is injected otherwise.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  drivers/acpi/apei/Kconfig |   3 ++
>  drivers/acpi/apei/einj.c  | 105 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 108 insertions(+)
> 
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index 6b18f8bc7be3..100c27beb581 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -11,6 +11,9 @@ config ACPI_APEI
>  	select PSTORE
>  	select UEFI_CPER
>  	depends on HAVE_ACPI_APEI
> +	imply CXL_BUS
> +	imply CXL_ACPI
> +	imply CXL_PORT

This goes away with the CONFIG_CXL_EINJ scheme proposed on patch1.

>  	help
>  	  APEI allows to report errors (for example from the chipset)
>  	  to the operating system. This improves NMI handling
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 330329ac2f1f..98d5e6d60a02 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -21,9 +21,11 @@
>  #include <linux/nmi.h>
>  #include <linux/delay.h>
>  #include <linux/mm.h>
> +#include <linux/pci.h>
>  #include <asm/unaligned.h>
>  
>  #include "apei-internal.h"
> +#include "../../cxl/cxl.h"

EINJ has no business knowing all of the details in cxl.h. In fact all
EINJ cares about is dport->dport_dev and dport->rch. I think just add
those to the calling convention, see below:

>  #undef pr_fmt
>  #define pr_fmt(fmt) "EINJ: " fmt
> @@ -627,6 +629,25 @@ static int available_error_type_show(struct seq_file *m, void *v)
>  
>  DEFINE_SHOW_ATTRIBUTE(available_error_type);
>  
> +static int cxl_einj_available_error_type(struct seq_file *m, void *v)
> +{
> +	int cxl_err, rc;
> +	u32 available_error_type = 0;
> +
> +	rc = einj_get_available_error_type(&available_error_type);
> +	if (rc)
> +		return rc;
> +
> +	for (int pos = 0; pos < ARRAY_SIZE(einj_cxl_error_type_string); pos++) {
> +		cxl_err = ACPI_EINJ_CXL_CACHE_CORRECTABLE << pos;
> +
> +		if (available_error_type & cxl_err)
> +			seq_puts(m, einj_cxl_error_type_string[pos]);
> +	}
> +
> +	return 0;
> +}
> +
>  static int validate_error_type(u64 type)
>  {
>  	u32 tval, vendor, available_error_type = 0;
> @@ -657,6 +678,68 @@ static int validate_error_type(u64 type)
>  	return 0;
>  }
>  
> +static int cxl_dport_get_sbdf(struct cxl_dport *dport, u64 *sbdf)
> +{
> +	struct pci_dev *pdev;
> +	struct pci_bus *pbus;
> +	struct pci_host_bridge *bridge;
> +	u64 seg = 0, bus;
> +
> +	if (!dev_is_pci(dport->dport_dev))
> +		return -EINVAL;
> +
> +	pdev = to_pci_dev(dport->dport_dev);
> +	pbus = pdev->bus;
> +	bridge = pci_find_host_bridge(pbus);
> +
> +	if (!bridge)
> +		return -ENODEV;
> +
> +	if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
> +		seg = bridge->domain_nr << 24;
> +
> +	bus = pbus->number << 16;
> +	*sbdf = seg | bus | pdev->devfn;
> +
> +	return 0;
> +}
> +
> +static int cxl_einj_inject_error(struct cxl_dport *dport, u64 type)

If you split this into cxl_einj_inject_error() and
cxl_einj_inject_rch_error() with following prototypes:

static int cxl_einj_inject_error(struct pci_dev *dport_dev, u64 type)
static int cxl_einj_inject_rch_error(u64 rcrb, u64 type)

...then you don't need any of the cxl.h definitions. The dev_is_pci()
and rch determination details can remain private to CXL.

> +{
> +	u64 param1 = 0, param2 = 0, param4 = 0;
> +	u32 flags;
> +	int rc;
> +
> +	/* Only CXL error types can be specified */
> +	if (type & ~CXL_ERROR_MASK || (type & ACPI5_VENDOR_BIT))
> +		return -EINVAL;
> +
> +	rc = validate_error_type(type);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * If dport is in restricted mode, inject a CXL 1.1 error,
> +	 * otherwise inject a CXL 2.0 error
> +	 */
> +	if (dport->rch) {
> +		if (dport->rcrb.base == CXL_RESOURCE_NONE)
> +			return -EINVAL;
> +
> +		param1 = (u64) dport->rcrb.base;
> +		param2 = 0xfffffffffffff000;
> +		flags = 0x2;
> +	} else {
> +		rc = cxl_dport_get_sbdf(dport, &param4);
> +		if (rc)
> +			return rc;
> +
> +		flags = 0x4;
> +	}
> +
> +	return einj_error_inject(type, flags, param1, param2, 0, param4);
> +}
> +
>  static int error_type_get(void *data, u64 *val)
>  {
>  	*val = error_type;
> @@ -668,6 +751,7 @@ static int error_type_set(void *data, u64 val)
>  {
>  	int rc;
>  
> +	/* CXL error types have to be injected from cxl debugfs */
>  	if (val & CXL_ERROR_MASK && !(val & ACPI5_VENDOR_BIT))
>  		return -EINVAL;
>  
> @@ -714,6 +798,7 @@ static int __init einj_init(void)
>  {
>  	int rc;
>  	acpi_status status;
> +	struct cxl_einj_ops cxl_ops;
>  	struct apei_exec_context ctx;
>  
>  	if (acpi_disabled) {
> @@ -793,6 +878,15 @@ static int __init einj_init(void)
>  				   einj_debug_dir, &vendor_flags);
>  	}
>  
> +	if (IS_REACHABLE(CONFIG_CXL_ACPI) || IS_REACHABLE(CONFIG_CXL_PORT)) {
> +		cxl_ops = (struct cxl_einj_ops) {
> +			.einj_type = cxl_einj_available_error_type,
> +			.einj_inject = cxl_einj_inject_error,
> +		};
> +
> +		cxl_einj_set_ops_cbs(&cxl_ops);
> +	}

This goes away with the new Kconfig dependency scheme.

> +
>  	pr_info("Error INJection is initialized.\n");
>  
>  	return 0;
> @@ -810,8 +904,18 @@ static int __init einj_init(void)
>  
>  static void __exit einj_exit(void)
>  {
> +	struct cxl_einj_ops cxl_ops;
>  	struct apei_exec_context ctx;
>  
> +	if (IS_REACHABLE(CONFIG_CXL_ACPI) || IS_REACHABLE(CONFIG_CXL_PORT)) {
> +		cxl_ops = (struct cxl_einj_ops) {
> +			.einj_type = NULL,
> +			.einj_inject = NULL,
> +		};
> +
> +		cxl_einj_set_ops_cbs(&cxl_ops);
> +	}
> +
>  	if (einj_param) {
>  		acpi_size size = (acpi5) ?
>  			sizeof(struct set_error_type_with_address) :
> @@ -832,4 +936,5 @@ module_exit(einj_exit);
>  
>  MODULE_AUTHOR("Huang Ying");
>  MODULE_DESCRIPTION("APEI Error INJection support");
> +MODULE_IMPORT_NS(CXL);

EINJ never references a CXL symbol in the new proposed scheme.