Re: [PATCH v12 2/3] cxl/core, EINJ: Add EINJ CXL debugfs files and EINJ helper functions

[Ben Cheatham <benjamin.cheatham@amd.com>]

On 2/14/24 8:25 PM, Dan Williams wrote:
> Ben Cheatham wrote:
>> Implement CXL helper functions in the EINJ module for getting/injecting
>> available CXL protocol error types and export them to sysfs under
>> kernel/debug/cxl.
>>
>> The kernel/debug/cxl/einj_types file will print the available CXL
>> protocol errors in the same format as the available_error_types
>> file provided by the EINJ module. The
>> kernel/debug/cxl/$dport_dev/einj_inject is functionally the same as the
>> error_type and error_inject files provided by the EINJ module, i.e.:
>> writing an error type into $dport_dev/einj_inject will inject said error
>> type into the CXL dport represented by $dport_dev.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> [..]
>> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
>> index 67998dbd1d46..d1fc3ce31fbb 100644
>> --- a/drivers/cxl/Kconfig
>> +++ b/drivers/cxl/Kconfig
>> @@ -157,4 +157,16 @@ config CXL_PMU
>>  	  monitoring units and provide standard perf based interfaces.
>>  
>>  	  If unsure say 'm'.
>> +
>> +config CXL_EINJ
>> +	bool "CXL Error INJection Support"
>> +	default ACPI_APEI_EINJ
>> +	depends on ACPI_APEI_EINJ = CXL_BUS
> 
> So I do not see CONFIG_CXL_EINJ used anywhere, not in a Makefile, not in
> a header file. My expectation is that if this variable is not set then
> no symbols from einj.ko are consumed by cxl_core.ko.
> 

Yeah, you're right. More on this below.

>> +	help
>> +	  Support for CXL protocol Error INJection through debugfs/cxl.
>> +	  Availability and which errors are supported is dependent on
>> +	  the host platform. Look to ACPI v6.5 section 18.6.4 and kernel
>> +	  EINJ documentation for more information.
>> +
>> +	  If unsure say 'n'
>>  endif
> [..]
>> diff --git a/include/linux/einj-cxl.h b/include/linux/einj-cxl.h
>> new file mode 100644
>> index 000000000000..92c0e2e37ad9
>> --- /dev/null
>> +++ b/include/linux/einj-cxl.h
>> @@ -0,0 +1,40 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * CXL protocol Error INJection support.
>> + *
>> + * Copyright (c) 2023 Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Author: Ben Cheatham <benjamin.cheatham@amd.com>
>> + */
>> +#ifndef CXL_EINJ_H
>> +#define CXL_EINJ_H
>> +
>> +#include <linux/pci.h>
>> +
>> +#if IS_ENABLED(CONFIG_ACPI_APEI_EINJ)
> 
> Per above this needs to be IS_ENABLED(CONFIG_CXL_EINJ), otherwise what's
> the point of the config symbol?
> 

So I've tried changing this to IS_ENABLED(CONFIG_CXL_EINJ) and always get redefinition
errors that I can't figure out how to get around cleanly. I should've elaborated more
in the last revision, but part of changing the dependency rule from ACPI_APEI_EINJ >= CXL_BUS
to ACPI_APEI_EINJ = CXL_BUS is that the above guard stays as IS_ENABLED(CONFIG_ACPI_APEI_EINJ).

I'm pretty sure the only thing this symbol is doing is enforcing the above dependency.
I would love to be able to remove it at this point, but doing so would require moving the
dependency to either the EINJ or CXL core modules, which sounds worse. I could implement
one of the other solutions I outlined last revision, but I don't particularly like any of
those (and I know you don't either :)).

I think the solution here is to move the einj_cxl functions into a new file, gate that
file by CONFIG_CXL_EINJ (or change the name to CONFIG_EINJ_CXL to match einj-cxl.h),
and add declarations of the functions in the EINJ module used by said functions to
drivers/acpi/apei/apei-internal.h. I'm not sure of another approach at this point,
but if you have suggestions I'd be very happy to hear them!

Thanks,
Ben

>> +int einj_cxl_available_error_type_show(struct seq_file *m, void *v);
>> +int einj_cxl_inject_error(struct pci_dev *dport_dev, u64 type);
>> +int einj_cxl_inject_rch_error(u64 rcrb, u64 type);
>> +bool einj_is_initialized(void);
>> +#else // !IS_ENABLED(CONFIG_ACPI_APEI_EINJ)
>> +static inline int einj_cxl_available_error_type_show(struct seq_file *m,
>> +						     void *v)
>> +{
>> +	return -ENXIO;
>> +}
>> +
>> +static inline int einj_cxl_inject_error(struct pci_dev *dport_dev, u64 type)
>> +{
>> +	return -ENXIO;
>> +}
>> +
>> +static inline int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
>> +{
>> +	return -ENXIO;
>> +}
>> +
>> +static inline bool einj_is_initialized(void) { return false; }
>> +#endif // CONFIG_ACPI_APEI_EINJ
>> +
>> +#endif // CXL_EINJ_H
>> -- 
>> 2.34.1
>>
>>
> 
> 
>