Re: [PATCH 20/23] cxl/port: Introduce a port driver

[Dan Williams <dan.j.williams@intel.com>]

On Tue, Nov 23, 2021 at 3:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Christoph, since he has opinions about portdrv;
> Greg, Rafael, since they have good opinions about sysfs structure]
>
> On Tue, Nov 23, 2021 at 02:36:32PM -0800, Dan Williams wrote:
> > On Tue, Nov 23, 2021 at 2:03 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > [..]
> > > > I hope this driver is not modeled on the PCI portdrv.  IMO, that
> > > > was a design error, and the "port service drivers" (PME,
> > > > hotplug, AER, etc) should be directly integrated into the PCI
> > > > core instead of pretending to be independent drivers.
> > >
> > > I'd like to understand a bit about why you think it was a design
> > > error. The port driver is intended to be a port services driver,
> > > however I see the services provided as quite different than the
> > > ones you mention. The primary service cxl_port will provide from
> > > here on out is the ability to manage HDM decoder resources for a
> > > port. Other independent drivers that want to use HDM decoder
> > > resources would rely on the port driver for this.
> > >
> > > It could be in CXL core just the same, but I don't see too much of
> > > a benefit since the code would be almost identical. One nice
> > > aspect of having the port driver outside of CXL core is it would
> > > allow CXL devices which do not need port services (type-1 and
> > > probably type-2) to not need to load the port driver. We do not
> > > have examples of such devices today but there's good evidence they
> > > are being built. Whether or not they will even want CXL core is
> > > another topic up for debate however.
> > >
> > > I admit Dan and I did discuss putting this in either its own port
> > > driver, or within core as a port driver. My inclination is, less
> > > is more in CXL core; but perhaps you will be able to change my
> > > mind.
> >
> > No, I don't think Bjorn is talking about whether the driver is
> > integrated into cxl_core.ko vs its own cxl_port.ko. IIUC this goes
> > back to the original contention about have /sys/bus/cxl at all:
> >
> > https://lore.kernel.org/r/CAPcyv4iu8D-hJoujLXw8a4myS7trOE1FcUhESLB_imGMECVfrg@mail.gmail.com
>
> That question was about whether we want the same physical device to be
> represented both in the /sys/bus/pci hierarchy and the /sys/bus/cxl
> hierarchy.  That still seems a little weird to me, but I don't know
> enough about the CXL constraints to really object to it.
>
> My question here is more about whether you're going to use something
> like the pcie_port_service_register() model for supporting multiple
> drivers attached to the same physical device.
>
> The PCIe portdrv creates a /sys/bus/pci_express hierarchy parallel to
> the /sys/bus/pci hierarchy.  The pci_express hierarchy has a "device"
> for every service (hotplug, AER, DPC, PME, etc) (see
> pcie_device_init()).  This device creation is quite complicated and
> depends on whether the Port advertises a Capability, whether the
> platform has granted control to the OS, whether support is compiled
> in, etc.
>
> I think that was a mistake because these hotplug, AER, DPC, PME
> "devices" are not independent things.  They are optional features that
> can be added to a variety of devices, and those devices might have
> their own drivers.  For example, we want to write drivers for
> vendor-specific functionality like PMUs in switches, but we can't do
> that cleanly because portdrv claims them.
>
> The portdrv features are fully specified by the PCIe spec, so nothing
> is vendor-specific.  They share interrupts.  They share power state.
> They cannot be reset independently.  They are not addressable entities
> in the usual bus/device/function model.  They can't be removed or
> added like the underlying device can.  I wasn't there when they were
> designed, but from reading the spec, it seems like they were designed
> as optional features of a device, not as separate devices themselves.

Let me ask a clarifying question coming from the other direction that
resulted in the creation of the auxiliary bus architecture. Some
background. RDMA is a protocol that may run on top of Ethernet.
Consider the case where you have multiple generations of Ethernet
adapter devices, but they all support common RDMA functionality. You
only have the one PCI device to attach a unique Ethernet driver. What
is an idiomatic way to deploy a module that automatically loads and
attaches to the exported common functionality across adapters that
otherwise have a unique native driver for the hardware device?

Another example, the Native PCIe Enclosure Management (NPEM)
specification defines a handful of registers that can appear anywhere
in the PCIe hierarchy. How can you write a common driver that is
generically applicable to any given NPEM instance?

The auxiliary bus answer to those questions is to register an
auxiliary device to be driven by a common auxiliary driver across
hard-device generations from the same vendor or even across vendors.

For your example about a PCIe port PMU driver it ultimately requires
something to enumerate that capability and a library of code to
exercise that functionality. What is a more natural fit than a Linux
"device" and a Linux driver to coordinate attaching enabling code to a
standalone hardware capability?

PCIe portdrv may be awkward because there was never a real native
driver for the device to begin with and it all could have handled by
'pcie_portdriver' directly without registering more Linux devices, but
assigning self contained features to Linux devices otherwise seems a
useful idiom to me.

As for CXL, there is no analog of the PCIe portdrv pattern of just
having a device act as a multiplexer of features to other Linux
devices.