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

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

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>
> ---
> v1:
> - Change input param from cxlds to cxl_mbox
> - Move mbox_out declaration inline. (Jonathan)
> - Fix __counted_by() input. (Jonathan)
> - Return count for cxl_get_supported_features_count(). (Dan)
> - Remove goto from cxl_get_supported_features(). (Dan)
> - Return cxl_get_supported_feature_entry() directly. (Dan)
> - Drop user path enumeration. (Dan)
> - Move to the feature driver model. (Dan)
> - Add support for 0 feature data requested.
> - Add missing increment of feature data ptr during copy.
> ---
>  drivers/cxl/core/features.c |  28 +++++++
>  drivers/cxl/core/mbox.c     |   3 +-
>  drivers/cxl/cxl.h           |   2 +
>  drivers/cxl/features.c      | 146 +++++++++++++++++++++++++++++++++++-
>  include/cxl/features.h      |  32 ++++++++
>  5 files changed, 209 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index eb6eb191a32e..66a4b82910e6 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
>  #include <linux/device.h>
> +#include <cxl/mailbox.h>
>  #include "cxl.h"
>  #include "core.h"
>  
> @@ -69,3 +70,30 @@ struct cxl_features *cxl_features_alloc(struct cxl_mailbox *cxl_mbox,
>  	return ERR_PTR(rc);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_features_alloc, "CXL");
> +
> +struct cxl_feat_entry *
> +cxl_get_supported_feature_entry(struct cxl_features *features,
> +				const uuid_t *feat_uuid)
> +{
> +	struct cxl_feat_entry *feat_entry;
> +	struct cxl_features_state *cfs;
> +	int count;
> +
> +	cfs = dev_get_drvdata(&features->dev);
> +	if (!cfs)
> +		return ERR_PTR(-EOPNOTSUPP);

How could this function be called outside of the driver being enabled?

If the driver might not be enabled when this is called, what stops it
from being disabled immediately after this check?

Went looking for the caller of this function... none in this patch set.

If this is for the EDAC use case, that enabling had better be triggered
from a known context where these questions have a satisfactory answer.
As it stands I think you can just delete it, right?

This has me thinking that the EDAC integration should just be a feature
of the cxl_features_driver.

...that last sentence made me itch because I don't want to have to
untangle future sentences like: A new feature of the features driver
features a new Feature to complement the existing Features and other
features.

So I might need to ask you to be explicit about at least using capital
'Feature' when talking about a command, 'cxl_features_device' when
talking about the device and 'cxl_features_driver' when talking about
the driver.

> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 644add26975f..a5949312a4ab 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
[..]
> +static int cxl_get_supported_features(struct cxl_features_state *cfs)
> +{
> +	int remain_feats, max_size, max_feats, start, rc, hdr_size;
> +	struct cxl_mailbox *cxl_mbox = cfs->features->cxl_mbox;
> +	int feat_size = sizeof(struct cxl_feat_entry);
> +	struct cxl_mbox_get_sup_feats_in mbox_in;
> +	struct cxl_feat_entry *entry;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	struct cxl_mem_command *cmd;
> +	int count;
> +
> +	/* Get supported features is optional, need to check */
> +	cmd = cxl_find_feature_command(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
> +	if (!cmd)
> +		return -EOPNOTSUPP;
> +	if (!test_bit(cmd->info.id, cxl_mbox->feature_cmds))
> +		return -EOPNOTSUPP;

First, who is calling this function without knowing that features are
supported? Like how did we get here if something already did that first
level enumeration.

Otherwise, that looks like a lot of work just to ask the question, "did
the mailbox see CXL_MBOX_OP_GET_SUPPORTED_FEATURES" at init?

That could probably all be boiled down into an enum like:

enum cxl_features_capability {
	CXL_FEATURES_NONE,
	CXL_FEATURES_RO,
	CXL_FEATURES_RW,
};

...for tracking Feature command enumeration and optional write support.

The cxl_mem_commands organization is quirk of the CXL ioctl
implementation cxl_features is free to not copy it.

Longer term we need to think about how to make cxl_mailbox more
independent of the memory expander / cxl_pci use case. Whereby, perhaps
the consumer of the mailbox passes is in a callback that takes action on
each found command. Then we get out of this game of how to cache command
capabilities that are awkward fits depending on command set or driver
use case.

[..]