Re: [RFC PATCH v2 08/28] cxl/port: Introduce a port driver

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-11-04 13:04:13, Dan Williams wrote:
> On Thu, Nov 4, 2021 at 12:49 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-11-04 12:10:09, Dan Williams wrote:
> > > On Tue, Nov 2, 2021 at 9:58 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > >
> > > > On 21-11-01 20:31:03, Dan Williams wrote:
> > > > > On Mon, Nov 1, 2021 at 10:53 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > > >
> > > > > > On 21-10-29 18:37:36, Dan Williams wrote:
> > > > > > > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > > > > >
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> > > > > > > > index cf07ae6cea17..40b386aaedf7 100644
> > > > > > > > --- a/drivers/cxl/Makefile
> > > > > > > > +++ b/drivers/cxl/Makefile
> > > > > > > > @@ -1,5 +1,6 @@
> > > > > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > > > > >  obj-$(CONFIG_CXL_BUS) += core/
> > > > > > > > +obj-$(CONFIG_CXL_MEM) += cxl_port.o
> > > > > > >
> > > > > > > It feel odd that CONFIG_CXL_MEM builds cxl_port, why not have a
> > > > > > > CONFIG_CXL_PORT that is simply selected by CONFIG_CXL_MEM, or a
> > > > > > > CONFIG_CXL_PORT that defaults to the value of CONFIG_CXL_BUS?
> > > > > > >
> > > > > >
> > > > > > Can you help me understand when CONFIG_CXL_MEM is useful when
> > > > > > #CONFIG_CXL_PORT=n? I was unable to figure out such a case and so I tied the two
> > > > > > together.
> > > > >
> > > > > With a 'select' dependency it's impossible to have the
> > > > > CONFIG_CXL_PORT=n and CONFIG_CXL_MEM=m combination. The extra config
> > > > > symbol is for idiomatic (one config-symbol per module .ko) reasons to
> > > > > reflect the module dependency in the Kconfig.
> > > > >
> > > >
> > > > Should CXL_MEM also select CXL_PCI? I don't see a way CXL_MEM can work without
> > > > CXL_PCI.
> > >
> > > The "select" dependency should be an "I use services from module X"
> > > relationship. CXL_MEM uses port services and can't operate unless that
> > > module is present. CXL_MEM does not strictly need CXL_PCI especially
> > > since drivers/cxl/core/memdev.c abandoned all PCI refernces.
> >
> > The problem I see is cxl_mem devices have a soft dependency on cxl_pci. For
> > example what we discussed regard DVSEC range registers. My plan was if the
> > endpoint driver can't determine range registers aren't in use, it won't bind. It
> > can't make this determination without cxl_pci. DOE has a similar implication
> > though not fatal.
> 
> I think this gets back to my original preference of just making the
> port driver a part of the core rather than its own independent module
> for no real reason. In fact it makes the validation job harder because
> it expands the testing surface for different combinations of module
> loading and device binding.
> 
> For the specific issue of CXL_MEM dependencies on CXL_PCI, isn't
> CXL_PCI passing in cacheed data for CXL_MEM? CXL_MEM has no idea that
> CXL_PCI exists, but it certainly knows that CXL_PORT exists,

Thinking about it further, the current bar for entry in cxl_mem is there must as
least be component registers. If CXL_PCI doesn't do that, nothing will proceed,
so I think this is fine.

Since you brought it up, further down the road, I'm not opposed toward moving
the port driver into core, but for now I like it as the separate module. Some of
the topology host games we're playing now might have been avoidable, although I
think they're not a bad idea anyway.