Re: [RFC PATCH v2 09/20] fwctl/cxl: Add driver for CXL mailbox for handling CXL features commands

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Dec 09, 2024 at 12:35:14PM -0800, Dan Williams wrote:
> Jason Gunthorpe wrote:
> > On Thu, Dec 05, 2024 at 05:21:13PM -0800, Dan Williams wrote:
> > > Dave Jiang wrote:
> > > > Add a cxl fwctl driver to allow sending of CXL feature
> > > > commands from userspace through as ioctls. Create a driver skeleton for
> > > > initial setup.
> > > 
> > > I don't understand why fwctl support of the *existing* mailbox needs
> > > yet another sub-device and driver?
> > 
> > It is how to modprobe the fwctl driver independently from the main cxl
> > codebase.
> > 
> > You want the fwctl parts to be in their own loadable module even if
> > cxl core is built in, and the loadable module should auto load when
> > cxl is present in the system.
> > 
> > Spawning a new struct device is the best way to do this.
> 
> I agree with that, and CXL is already a multi-device subsystem for the
> same purpose. However, a device just for the few subcommands that go
> through fwctl feels too fine grained. I would say either move cxl_mbox
> to be its own device with a driver that does CXL native commands and CXL
> 'feature' commands via fwctl, or create a 'CXL features' device that
> support any functionality implemented by the 'features' command set of
> which fwctl is just the direct command submission ABI for that driver
> which may have other ABIs like EDAC associated with it.
> 
> The latter is similar to the proposed cxl_fwctl device, but focuses more
> on the 'CXL features' aspect and less on the 'this is a fwctl module'.

Well, the advantage of slicing it according to subsystem is you can
slice the code according to subsystem too. If you have a complex cxl
device then it can't just have a simple driver in fwctl, with its own
loadlable module.

I like the idea that the fwctl interfaces all have their own loadlable
modules, it gives the admin an option to blacklist the module if they
don't want fwctl for some reason without impacting anything else.

The original version of these patches used the auxbus, which is
intended for *exactly* this purpose. I think that got a bit lost with
the feedback to use the cxl bus instead?

Jason