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

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

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

Unlike pcieportdrv where the functionality is bounded to a single
device instance with relatively simpler hierarchical coordination of
the memory space and services. CXL interleaving is both a foreign
concept to the PCIE core and an awkward fit for the pci_bus_type
device model. CXL uses the cxl_bus_type and bus drivers to coordinate
CXL operations that have cross PCI device implications.

Outside of that I think the auxiliary device driver model, of which
the PCIE portdrv model is an open-coded form, is a useful construct
for separation of concerns. That said, I do want to hear more about
what trouble it has caused though to make sure that CXL does not trip
over the same issues longer term.

[..]
> > > +static void rescan_ports(struct work_struct *work)
> > > +{
> > > +   if (bus_rescan_devices(&cxl_bus_type))
> > > +           pr_warn("Failed to rescan\n");
> >
> > Needs some context.  A bare "Failed to rescan" in the dmesg log
> > without a clue about who emitted it is really annoying.
> >
> > Maybe you defined pr_fmt() somewhere; I couldn't find it easily.
> >
>
> I actually didn't know about pr_fmt() trick, so thanks for that. I'll improve
> this message to be more useful and contextual.

I am skeptical that this implementation of the workqueue properly
synchronizes with workqueue shutdown concerns, but I have not had a
chance to dig in too deep on this patchset.

Regardless, it is not worth reporting a rescan failure, because those
are to be expected here. The rescan attempts to rescan when a
constraint changes, there is no guarantee that all constraints are met
just because one constraint changed, so rescan failures
(device_attach() failures) are not interesting to report. The
individual driver probe failure reporting is sufficient.