Re: [PATCH v1 04/19] cxl: Add Get Supported Features command for kernel usage

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

Jonathan Cameron wrote:
> On Thu, 23 Jan 2025 16:30:17 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Dave Jiang wrote:
> > > CXL spec r3.1 8.2.9.6.1 Get Supported Features (Opcode 0500h)
> > > The command retrieve the list of supported device-specific features
> > > (identified by UUID) and general information about each Feature.
> > > 
> > > The driver will retrieve the feature entries in order to make checks and
> > > provide information for the Get Feature and Set Feature command. One of
> > > the main piece of information retrieved are the effects a Set Feature
> > > command would have for a particular feature.
> > > 
> > > Co-developed-by: Shiju Jose <shiju.jose@huawei.com>
> > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
[..]
> > This has me thinking that the EDAC integration should just be a feature
> > of the cxl_features_driver.
> 
> I'm not 100% sure I see what you mean here, but if you mean pushing
> the registration code into cxl_features.ko...
> 
> I replied to this in the other thread, but just to add a bit here.
> I think this creates a worse spiders web than we have today. Not
> all EDAC features we might add (e.g. device self test) have anything to do
> with get/set features.  I'm not sure we'll support that one in EDAC soon
> though it might make sense if there is generality with non CXL systems
> (I've been meaning to look into that.)

I came to the same conclusion and am now skeptical of cxl_features.ko vs
just extended the memdev ABI. We have long since shipped the expectation
that command passthrough is a compile time policy option. Each component
that wants a CXL-Features-FWCTL interface can just register one via CXL
core helpers. In this case it will be similar to what currently happens
with the fw_upload interface registered against the memdev, not a
cxl_fw_upload.ko driver.

> To me, the CXL features support is providing a service to some of the EDAC
> features, but they use other services from CXL drivers and potentially
> don't use CXL features at all.  For now we have the repair drivers
> that only use the features support for a tiny bit of what they do
> (tweaking device initiated aspects and detailed record generation
>  - if we even support those yet).
> 
> At some point we may have core CXL functionality depending on
> get/set features as well as there are things we definitely don't
> want in either EDAC or fwctl like metabits storage controls.
> So I'd be careful adding any thing to cxl_features beyond stuff
> that is definitely clustered with fwctl bit or helpers.
> 
> Soon I'd also expect us to add 'features' to fwctl interface
> that aren't using get/set features. I think you suggested that we
> could do this by making up some guids for them and pushing though
> this same interface.

Yes, although I hope the availability of FWCTL encourages new and
interesting device functionality to just be a Feature, so that we don't
have to play Linux GUID games. That might be what triggers the creation
of a query command because that could lead to future scenarios where the
kernel needs to dynamically mark a Feature as exclusive that was
previously allowed before the kernel knew about it.