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@xxxxxxxxx> 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@xxxxxxxxxx> >>> >>> 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
