Re: [PATCH v2 1/6] cxl/port: Fix CXL port initialization order when the subsystem is built-in

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 24 Oct 2024 09:19:17 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Tue, 22 Oct 2024 18:43:24 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > When the CXL subsystem is built-in the module init order is determined
> > > by Makefile order. That order violates expectations. The expectation is
> > > that cxl_acpi and cxl_mem can race to attach and that if cxl_acpi wins
> > > the race cxl_mem will find the enabled CXL root ports it needs and if
> > > cxl_acpi loses the race it will retrigger cxl_mem to attach via
> > > cxl_bus_rescan(). That only works if cxl_acpi can assume ports are
> > > enabled immediately upon cxl_acpi_probe() return. That in turn can only
> > > happen in the CONFIG_CXL_ACPI=y case if the cxl_port object appears
> > > before the cxl_acpi object in the Makefile.
> > > 
> > > Fix up the order to prevent initialization failures, and make sure that
> > > cxl_port is built-in if cxl_acpi is also built-in.
> > > 
> > > As for what contributed to this not being found earlier, the CXL
> > > regression environment, cxl_test, builds all CXL functionality as a
> > > module to allow to symbol mocking and other dynamic reload tests.  As a
> > > result there is no regression coverage for the built-in case.
> > > 
> > > Reported-by: Gregory Price <gourry@gourry.net>
> > > Closes: http://lore.kernel.org/20241004212504.1246-1-gourry@gourry.net
> > > Tested-by: Gregory Price <gourry@gourry.net>
> > > Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver")  
> > 
> > I don't like this due to likely long term fragility, but any other  
> 
> Please be specific about the fragility and how is this different than
> any other Makefile order fragility, like the many cases in
> drivers/Makefile/, or patches fixing up initcall order?

Sure, I don't like any of them ;) Mostly was having a grumpy day
rather than suggesting a change in this.

> 
> Now, an argument can be made that there are too many CXL sub-objects and
> more can be merged into a monolithic cxl_core object. The flipside of
> that is reduced testability, at least via symbol mocking techniques.
> Just look at the recent case where the fact that
> drivers/cxl/core/region.c is built into cxl_core.o rather than its own
> cxl_region.o object results in an in-line code change to support
> cxl_test [1]. There are tradeoffs.
Absolutely agree. 
> 
> > solution is probably more painful.  Long term we should really get
> > a regression test for these ordering issues in place in one of
> > the CIs.  
> 
> The final patch in this series does improve cxl_test's ability to catch
> regressions in module init order, and that ordering change did uncover a
> bug. The system works! 😅
That was indeed good to see!
> 
> Going further the test mode that is needed, in addition to QEMU
> emulation and cxl_test interface mocking, is kunit or similar [2]
> infrastructure for some function-scope unit tests.
> 
> [1]: http://lore.kernel.org/20241022030054.258942-1-lizhijian@fujitsu.com
> [2]: http://lore.kernel.org/170795677066.3697776.12587812713093908173.stgit@ubuntu
> 
Yeah, we have a long way to go on testing (common problem!)

Jonathan