Re: [PATCH v7 1/5] cxl/port: Add EINJ debugfs files and callback support

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

Ben Cheatham wrote:
[..]
> > This can be simplified. Have something like:
> > 
> > config CXL_EINJ
> > 	default CXL_BUS
> > 	depends on ACPI_APEI_EINJ && ACPI_APEI_EINJ=CXL_BUS
> > 	...
> > 
> > Then the documentation moves to Kconfig for how to enable this and the
> > CXL code can directly call into the EINJ module without worry.
> > 
> > It would of course need stubs like these in a shared header:
> > 
> > #ifdef CONFIG_CXL_EINJ
> > int cxl_einj_available_error_type(struct seq_file *m, void *v);
> > int cxl_einj_inject_error(struct cxl_dport *dport, u64 type);
> > #else
> > static inline int cxl_einj_available_error_type(struct seq_file *m, void *v)
> > {
> > 	return -ENXIO;
> > }
> > 
> > int cxl_einj_inject_error(struct cxl_dport *dport, u64 type)
> > {
> > 	return -ENXIO;
> > }
> > #endif
> > 
> 
> I had to go back and take a look, but I had a shared header in v5
> (link:
> https://lore.kernel.org/linux-cxl/20230926120418.0000575d@Huawei.com/).
> Jonathan recommended that I instead include cxl.h directly, but that
> was pretty much a completely different patch set at the time (and the
> header was under include/linux/). That being said, I agree that a
> header under drivers/cxl would make much more sense here.

I agree with Jonathan that there are still cases where it makes sense to
do the relative include thing, but cxl_pmu is an intimate extenstion of
the CXL subsystem that just happens to live in a another directory. This
EINJ support is a handful of functions to communicate between modules
with no need for EINJ to understand all the gory details in cxl.h.

[..]
> >> +Date:               November, 2023
> >> +KernelVersion:      v6.8
> >> +Contact:    linux-cxl@vger.kernel.org
> >> +Description:
> >> +            (WO) Writing an integer to this file injects the corresponding
> >> +            CXL protocol error into dportY (integer to type mapping is
> >> +            available by reading from einj_types). If the dport was
> >> +            enumerated in RCH mode, a CXL 1.1 error is injected, otherwise
> >> +            a CXL 2.0 error is injected. This file is only visible if
> >> +            CONFIG_ACPI_APEI_EINJ is enabled, and the EINJ module must
> >> +            be able to reach one (or both) of the CXL_ACPI or CXL_PORT
> >> +            modules to be functional.
> > 
> > Similar comments about dropping these details that can just be solved in
> > Kconfig.
> > 
> > This is next comment is on EINJ ABI, but you can skip it just to
> > maintain momentum with the status quo. Why require the user to do the
> > string to integer conversion? Seems like a small matter of programming
> > to allow:
> > 
> > echo "CXL.cache Protocol Correctable" > einj_inject
> > 
> > ...to do the right thing. That probably makes scripts more readable as
> > well.
> > 
> 
> That's a good point. I can do that, but I think it may be better to keep the
> consistency with the EINJ module to simplify things for end users. If you feel
> that isn't a big enough concern I can go ahead and modify it.

Oh, definitely keep the old style as well. I was thinking of something
like:

echo "CXL.cache Protocol Correctable" > einj_inject
echo "0x1000" > einj_inject

...having the same result. Up to you though, I will still take the
series if only the integer way works.

[..]
> >> +	snprintf(dir_name, 31, "dport%d", dport->port_id);
> > 
> > How about dev_name(dport->dport_dev) rather than the dynamic name?
> > 
> > The other benefit of this is that the dport_dev names are unique, so you
> > can move the einj_inject file to:
> > 
> > /sys/kernel/debug/cxl/$dport_dev/einj_inject
> > 
> 
> I didn't realize the dport names were also unique. I'll go ahead and do that instead.

Yeah, you can assume that all devices that share a bus must have unique
names so that /sys/bus/$bus/devices can list all of them without
name collisions.