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

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

On 12/8/23 2:35 PM, Ben Cheatham wrote:
> 
> 
> 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.
> 

Ok so I took a look at implementing this. I don't think this solution ends up having
the intended behavior. Using a shared header and the Kconfig rules above introduces
a dependency on the EINJ module, which I was trying to avoid by using the callbacks
since I don't think the CXL core should fail to load if the EINJ module fails.
So, unless you have any other suggestions, I'll use the Kconfig rules above but keep
the callbacks (and also change the EINJ module to use IS_REACHABLE(CONFIG_CXL_EINJ) instead
of IS_REACHABLE(CONFIG_CXL_ACPI) || IS_REACHABLE(CONFIG_CXL_PORT)). I may also just
be doing something wrong (most likely due to late Friday brain fog), so let me know if
I got something wrong here.

Thanks,
Ben

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