On Tue, Nov 23, 2021 at 2:03 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> 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@xxxxxxxxxxxxxx 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.
