Re: [PATCH v1 07/19] cxl/mbox: Add GET_FEATURE mailbox command

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

Dave Jiang wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Add support for GET_FEATURE mailbox command.
> 
> CXL spec 3.1 section 8.2.9.6 describes optional device specific features.
> The settings of a feature can be retrieved using Get Feature command.
> CXL spec 3.1 section 8.2.9.6.2 describes Get Feature command.
> 
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v1:
> - pass in cxl_mbox instead of cxlds (Dan)
> - Move to the feature driver model. (Dan)
> ---
>  drivers/cxl/core/features.c | 74 +++++++++++++++++++++++++++++++++++++
>  drivers/cxl/features.c      |  6 +--
>  include/cxl/features.h      | 27 ++++++++++++++
>  3 files changed, 102 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 66a4b82910e6..ab9386b53a95 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -4,6 +4,7 @@
>  #include <cxl/mailbox.h>
>  #include "cxl.h"
>  #include "core.h"
> +#include "cxlmem.h"
>  
>  #define CXL_FEATURE_MAX_DEVS 65536
>  static DEFINE_IDA(cxl_features_ida);
> @@ -97,3 +98,76 @@ cxl_get_supported_feature_entry(struct cxl_features *features,
>  	return ERR_PTR(-ENOENT);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_get_supported_feature_entry, "CXL");
> +
> +bool cxl_feature_enabled(struct cxl_features_state *cfs, u16 opcode)
> +{
> +	struct cxl_mailbox *cxl_mbox = cfs->features->cxl_mbox;
> +	struct cxl_mem_command *cmd;
> +
> +	cmd = cxl_find_feature_command(opcode);
> +	if (!cmd)
> +		return false;
> +
> +	return test_bit(cmd->info.id, cxl_mbox->feature_cmds);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_feature_enabled, "CXL");
> +
> +size_t cxl_get_feature(struct cxl_features *features, const uuid_t feat_uuid,
> +		       enum cxl_get_feat_selection selection,
> +		       void *feat_out, size_t feat_out_size, u16 offset,

Is @feat_out guaranteed to be a kernel pointer, or might it be an __user
pointer?

Shouldn't @feat_uuid be a 'uuid_t *' rather than a 'uuid_t'?

> +		       u16 *return_code)
> +{
> +	size_t data_to_rd_size, size_out;
> +	struct cxl_features_state *cfs;
> +	struct cxl_mbox_get_feat_in pi;
> +	struct cxl_mailbox *cxl_mbox;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	size_t data_rcvd_size = 0;
> +	int rc;
> +
> +	if (return_code)
> +		*return_code = CXL_MBOX_CMD_RC_INPUT;
> +
> +	cfs = dev_get_drvdata(&features->dev);
> +	if (!cfs)
> +		return 0;
> +
> +	if (!cxl_feature_enabled(cfs, CXL_MBOX_OP_GET_FEATURE))
> +		return 0;

Per previous feedback, just make the caller responsible for knowing this
in advance, not checking every call. With that gone this function loses
its dependency on 'struct cxl_features' and 'struct cxl_features_state'
and can take 'struct cxl_mbox' directly.

If for some reason the caller did not know that Get Feature was missing
the device will still fail it anyway, so that boilerplate is not serving
any useful purpose.