Re: [PATCH v4 02/15] cxl: Add Get Supported Features command for kernel usage

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

On 2/7/25 11:13 PM, Li Ming wrote:
> On 2/8/2025 7:37 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. The retrieved Feature
>> entries are stored in the cxl_mailbox context.
>>
>> The setup of Features is initiated via devm_cxl_setup_features() during the
>> pci probe function before the cxl_memdev is enumerated.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> [...]
>> +static struct cxl_feat_entries *
>> +get_supported_features(struct cxl_features_state *cxlfs)
>> +{
>> +	int remain_feats, max_size, max_feats, start, rc, hdr_size;
>> +	struct cxl_mailbox *cxl_mbox = &cxlfs->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;
>> +
>> +	count = cxl_get_supported_features_count(cxl_mbox);
>> +	if (count <= 0)
>> +		return NULL;
>> +
>> +	struct cxl_feat_entries *entries __free(kvfree) =
>> +		kvmalloc(struct_size(entries, ent, count), GFP_KERNEL);
>> +	if (!entries)
>> +		return NULL;
>> +
>> +	struct cxl_mbox_get_sup_feats_out *mbox_out __free(kvfree) =
>> +		kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
>> +	if (!mbox_out)
>> +		return NULL;
>> +
>> +	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->ent;
>> +
>> +	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 ERR_PTR(rc);
> Should return NULL here?

Thanks for the catch. I missed that one.

DJ
>> +
>> +		if (mbox_cmd.size_out <= hdr_size)
>> +			return NULL;
>> +
>> +		/*
>> +		 * Make sure retrieved out buffer is multiple of feature
>> +		 * entries.
>> +		 */
>> +		retrieved = mbox_cmd.size_out - hdr_size;
>> +		if (retrieved % feat_size)
>> +			return NULL;
>> +
>> +		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 NULL;
>> +
>> +		memcpy(entry, mbox_out->ents, retrieved);
>> +		entry += num_entries;
>> +		/*
>> +		 * 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);
>> +
>> +	entries->num_features = count;
>> +
>> +	return no_free_ptr(entries);
>> +}
>> +
>> +static void free_cxlfs(void *_cxlfs)
>> +{
>> +	struct cxl_features_state *cxlfs = _cxlfs;
>> +	struct cxl_dev_state *cxlds = cxlfs->cxlds;
>> +
>> +	cxlds->cxlfs = NULL;
>> +	kvfree(cxlfs->entries);
>> +	kfree(cxlfs);
>> +}
>> +
>> +/**
>> + * devm_cxl_setup_features() - Allocate and initialize features context
>> + * @cxlds: CXL device context
>> + *
>> + * Return 0 on success or -errno on failure.
>> + */
>> +int devm_cxl_setup_features(struct cxl_dev_state *cxlds)
>> +{
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	int rc;
>> +
>> +	if (cxl_mbox->feat_cap < CXL_FEATURES_RO)
>> +		return -ENODEV;
>> +
>> +	struct cxl_features_state *cxlfs __free(kfree) =
>> +		kzalloc(sizeof(*cxlfs), GFP_KERNEL);
>> +	if (!cxlfs)
>> +		return -ENOMEM;
>> +
>> +	cxlfs->cxlds = cxlds;
>> +
>> +	cxlfs->entries = get_supported_features(cxlfs);
>> +	if (!cxlfs->entries)
>> +		return -ENOMEM;
> 
> It will be a problem if the above cxl_internal_send_cmd() in get_supported_features() returns a "ERR_PTR(rc)". My understanding is the cxl_internal_send_cmd() should return a NULL in failure cases.
> 
> Other looks good to me.
> 
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> 
>