Re: [PATCH v8 2/5] ACPI, APEI, EINJ: Add wrapper __init function

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

On 12/19/23 2:48 PM, Dan Williams wrote:
> Jonathan Cameron wrote:
>> On Mon, 18 Dec 2023 15:59:12 -0800
>> Dan Williams <dan.j.williams@intel.com> wrote:
>>
>>> Ben Cheatham wrote:
>>>> The CXL core module should be able to load regardless of whether the
>>>> EINJ module initializes correctly. Instead of porting the EINJ module to
>>>> a library module, add a wrapper __init function around einj_init() to  
>>>
>>> Small quibble with this wording... the larger EINJ module refactoring
>>> would be separating module_init() from EINJ probe(). As is this simple
>>> introduction of an einit_init() wrapper *is* refactoring this module to
>>> be used as a module dependency.
>>>
>>>> pin the EINJ module even if it does not initialize correctly. This
>>>> should be fine since the EINJ module is only ever unloaded manually.
>>>>
>>>> One note: since the CXL core will be calling into the EINJ module
>>>> directly, even though it may not have initialized, all CXL helper
>>>> functions *have* to check if the EINJ module is initialized before
>>>> doing any work.  
>>>
>>> Another small quibble here, perhaps s/may not have initialized/may not
>>> have successfully initialized/? Because initialization will have
>>> definitely completed one way or the other, but callers need to abort if
>>> it completed in error.
>>>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
>>>
>>> Did Jonathan really get in and review this new patch in the series
>>> before me? If yes, apologies I missed it, if no I think it is best
>>> practice to not carry forward series Reviewed-by's if new patches appear
>>> in the series between revisions.
>>
>> I'm not keen on the solution as it's esoteric and to me seems fragile.
>> I've looked at discussion on v7 and can see why you ended up with this
>> but I'd have preferred to see the 'violent' approach :)
> 
> The issue though is similar to the argument for the creation of the
> ACPI0017 device for CXL, there is not a great place to hang the einj
> device-driver.
> 
> However, since einj has no legacy "auto-load" behavior, I think it is
> not a lot of code to have einj's module_init() do something like this:
> 
> 	einj_dev = platform_device_register_full(&einj_dev_info);
> 	platform_driver_register(&einj_driver);
> 
> Ben, you want to give that a shot? Jonathan is right that my proposed
> hack is *a* solution but probably not *the* solution where this should
> end up.

I can take a look. I won't be able to get to it until around the new year
since I'm vacation at the moment. I'll also respond take a look at the
rest of your review around then.

Thanks,
Ben