Message-ID: <6786fa74bb11d_20f329479@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 14 Jan 2025 15:59:48 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>, Dan Williams
	<dan.j.williams@...el.com>, <alejandro.lucero-palau@....com>,
	<linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<martin.habets@...inx.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<dave.jiang@...el.com>
Subject: Re: [PATCH v8 02/27] sfc: add cxl support using new CXL API

Alejandro Lucero Palau wrote:
[..]
> >> +#ifdef CONFIG_SFC_CXL
> >> +MODULE_SOFTDEP("pre: cxl_core cxl_port cxl_acpi cxl-mem");
> > No, endpoint drivers should not need softdep for cxl core modules.
> > Primarily because this does nothing to ensure platform CXL
> > capability-enumeration relative to PCI driver loading, and because half
> > of those softdeps in that statement are redundant or broken.
> >
> > - cxl_core is already a dependency due to link time dependencies
> > - cxl_port merely being loaded does nothing to enforce that port probing
> >    is complete by the time the driver loads. Instead the driver needs to
> >    use EPROBE_DEFER to wait for CXL enumeration, or it needs to use the
> >    scheme that cxl_pci uses which is register a memdev and teach userspace
> >    to wait for that memdev attaching to its driver event as the "CXL memory
> >    is now available" event.
> > - cxl_acpi is a platform specific implementation detail. When / if a
> >    non-ACPI platform ever adds CXL support it would be broken if every
> >    endpoint softdep line needed to then be updated
> > - cxl-mem is misspelled cxl_mem and likely is not having any effect.
> >
> > In short, if you delete this line and something breaks then it needs to
> > be fixed in code and not module dependencies.
> 
> 
> It has been a while since I worked on this part, so I need to put my 
> mind back, but with the right softdeps the driver initialization does 
> not fail due to the CXL modules not being loaded yet.
> 
> With a softdep for cxl_port, which is the problem here, and with 
> cxl_port having a dependency for cxl_core, I would say everything the 
> driver needs should be there at the time the sfc driver is loaded. I 
> agree cxl_acpi could be a problem because it is architecture dependent, 
> and I need to see the cxl_mem dependency which is obviously unclear 
> after you spotting the typo.
> 
> In other words, I need to think about all this again and perform some 
> testing. This was implemented after the problems you solved regarding 
> the loading dependencies and enumeration delays, and from our discussion 
> then, I though EPROBE_DEFER was not needed for solving this.

The loading dependencies were only to fix the fact that cxl_acpi was not
able to assert that the top-level ports that it registered were active
before cxl_acpi_probe() returned.

That is different from the endpoint case which has no idea when or if
the cxl_port hierarchy is going to come online, and should not care.

An endpoint only has 2 choices:

1/ return EPROBE_DEFER until an endpoint-port object has a cxl port
   topology to attach. This is subject to arbitrary probe deferral
   timeouts.

2/ use the intermediate object approach like the cxl_mem driver that
   enjoys the cxl_acpi driver re-triggering cxl_mem_probe() whenever the
   cxl_port topology comes online. This solution can handle indefinite
   initialization waits but it means the CXL functionality of the driver
   needs to be enabled dynamically which is more complicated.

...I'll go check v9 to see what might have changed here.

Powered by blists - more mailing lists