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@xxxxxxxxxx/).
> 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@xxxxxxxxxxxxxxx
> >> +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.
