Re: [PATCH v3 03/16] cxl: Add Get Supported Features command for kernel usage

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

On 2/6/25 10:42 PM, Li Ming wrote:
> On 2/5/2025 6:03 AM, Dave Jiang wrote:
>> CXL spec r3.2 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.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v3:
>> - Use struct_size() instead of open coding the calculation. (Jonathan)
>> ---
>>  drivers/cxl/features.c | 138 +++++++++++++++++++++++++++++++++++++++++
>>  include/cxl/features.h |  57 +++++++++++++++++
>>  2 files changed, 195 insertions(+)
>>
>> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
>> index 958e4828a58d..4d8c603121f0 100644
>> --- a/drivers/cxl/features.c
>> +++ b/drivers/cxl/features.c
>> @@ -7,6 +7,139 @@
> [...]
>> +static int get_supported_features(struct cxl_memdev *cxlmd,
>> +				  struct cxl_features_state *cxlfs)
>> +{
>> +	int remain_feats, max_size, max_feats, start, rc, hdr_size;
>> +	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->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;
>> +	int count;
>> +
>> +	if (cxlfs->cap < CXL_FEATURES_RO)
>> +		return -EOPNOTSUPP;
>> +
>> +	count = cxl_get_supported_features_count(cxl_mbox);
>> +	if (count == 0)
>> +		return -ENOENT;
>> +	if (count < 0)
>> +		return -ENXIO;
>> +
>> +	struct cxl_feat_entry *entries __free(kvfree) =
>> +		kvmalloc(count * sizeof(*entries), GFP_KERNEL);
>> +	if (!entries)
>> +		return -ENOMEM;
>> +
>> +	struct cxl_mbox_get_sup_feats_out *mbox_out __free(kvfree) =
>> +		kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
>> +	if (!mbox_out)
>> +		return -ENOMEM;
>> +
>> +	hdr_size = struct_size(mbox_out, ents, 0);
>> +	max_size = cxl_mbox->payload_size - hdr_size;
>> +	/* max feat entries that can fit in mailbox max payload size */
>> +	max_feats = max_size / feat_size;
>> +	entry = entries;
>> +
>> +	start = 0;
>> +	remain_feats = count;
>> +	do {
>> +		int retrieved, alloc_size, copy_feats;
>> +		int num_entries;
>> +
>> +		if (remain_feats > max_feats) {
>> +			alloc_size = struct_size(mbox_out, ents, max_feats);
>> +			remain_feats = remain_feats - max_feats;
>> +			copy_feats = max_feats;
>> +		} else {
>> +			alloc_size = struct_size(mbox_out, ents, remain_feats);
>> +			copy_feats = remain_feats;
>> +			remain_feats = 0;
>> +		}
>> +
>> +		memset(&mbox_in, 0, sizeof(mbox_in));
>> +		mbox_in.count = cpu_to_le32(alloc_size);
>> +		mbox_in.start_idx = cpu_to_le16(start);
>> +		memset(mbox_out, 0, alloc_size);
>> +		mbox_cmd = (struct cxl_mbox_cmd) {
>> +			.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +			.size_in = sizeof(mbox_in),
>> +			.payload_in = &mbox_in,
>> +			.size_out = alloc_size,
>> +			.payload_out = mbox_out,
>> +			.min_out = hdr_size,
>> +		};
>> +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		if (mbox_cmd.size_out <= hdr_size)
>> +			return -ENXIO;
>> +
>> +		/*
>> +		 * Make sure retrieved out buffer is multiple of feature
>> +		 * entries.
>> +		 */
>> +		retrieved = mbox_cmd.size_out - hdr_size;
>> +		if (retrieved % feat_size)
>> +			return -ENXIO;
>> +
>> +		num_entries = le16_to_cpu(mbox_out->num_entries);
>> +		/*
>> +		 * If the reported output entries * defined entry size !=
>> +		 * retrieved output bytes, then the output package is incorrect.
>> +		 */
>> +		if (num_entries * feat_size != retrieved)
>> +			return -ENXIO;
>> +
>> +		memcpy(entry, mbox_out->ents, retrieved);
>> +		entry++;
> 
> This memcpy() copies multiple entries from mbox_out->ents to entry, this ''entry++" should be "entry += num_entries"?

Thanks for finding this Ming. Appreciate the review. Fixed in v4.

DJ

> 
> 
> Ming
> 
>> +		/*
>> +		 * If the number of output entries is less than expected, add the
>> +		 * remaining entries to the next batch.
>> +		 */
>> +		remain_feats += copy_feats - num_entries;
>> +		start += num_entries;
>> +	} while (remain_feats);
>> +
>> +	cxlfs->num_features = count;
>> +	cxlfs->entries = no_free_ptr(entries);
>> +	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
>> +					cxlfs->entries);
>> +}
>> +
>