Re: [PATCH v6 11/14] cxl: Add support to handle user feature commands for set feature

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 18 Feb 2025 15:54:40 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add helper function to parse the user data from fwctl RPC ioctl and
> send the parsed input parameters to cxl_set_feature() call.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v6:
> - Add set features bits checking bits from two patches ago. (Saeed)
> - Adjust for embedded input payload

Follow on from earlier.  Now I see where op is used.
I think we can do better however.

>  
> +static struct cxl_feat_entry *
> +get_support_feature_info(struct cxl_features_state *cxlfs,
> +			 const struct fwctl_rpc_cxl *rpc_in)
> +{
> +	struct cxl_feat_entry *feat;
> +	uuid_t *uuid;
> +
> +	if (rpc_in->op_size < sizeof(uuid))
> +		return ERR_PTR(-EINVAL);
> +
> +	uuid = (uuid_t *)rpc_in->op;

hmm. So here you use the op, but we know this a set features, so
much like in previous patch we could use

rpc_in->set_feat_in->uuid

That is relying on the fact we know this is being called on a
set feature which the name doesn't imply. So maybe rename?

Or.. check rpc_in->opcode and grab it from rpc_in->get_feat_in->uuid or
rpc_in->set_feat_in->uuid as appropriate.
and error out on anything else as we don't 'know' for sure there
is a uuid at that location.

> +
> +	for (int i = 0; i < cxlfs->entries->num_features; i++) {
> +		feat = &cxlfs->entries->ent[i];
> +		if (uuid_equal(uuid, &feat->uuid))
> +			return feat;
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +