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

[Dave Jiang <dave.jiang@intel.com>]

On 2/19/25 11:12 AM, Jonathan Cameron wrote:
> 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.

Yes that makes sense. I'll drop the flex array in the input data struct. 

> 
>> +
>> +	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);
>> +}
>> +
> 
> 
>