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

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

On 12/8/23 12:01 PM, Dan Williams wrote:
> 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.
> 

All right that makes sense. Thanks for the clarification.

> [..]
>>>> +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.
> 

Sounds good. If I get the time I'll add in the string version as well.

> [..]
>>>> +	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.

Yeah I was thinking that dev_name(dport->dport_dev) would give a name like dportX for some reason
and realized what that would actually do about 30 mins after I sent out the email :/.

Thanks,
Ben