On 21-11-23 14:36:32, Dan Williams wrote: > 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. Yeah, we discussed this. Please do check that. I'm happy to send out v2 with all of Jonathan's fixes first, so you don't have to duplicate effort with what he has already uncovered. > > 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. Agreed.
