RE: [RFC PATCH v2 02/20] cxl: Add Get Supported Features command for kernel usage

[Shiju Jose <shiju.jose@huawei.com>]

Hi Dan,

>-----Original Message-----
>From: Dan Williams <dan.j.williams@intel.com>
>Sent: 06 December 2024 00:33
>To: Dave Jiang <dave.jiang@intel.com>; linux-cxl@vger.kernel.org
>Cc: dan.j.williams@intel.com; ira.weiny@intel.com; vishal.l.verma@intel.com;
>alison.schofield@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; dave@stgolabs.net; jgg@nvidia.com; Shiju
>Jose <shiju.jose@huawei.com>
>Subject: Re: [RFC PATCH v2 02/20] cxl: Add Get Supported Features command
>for kernel usage
>
>Dave Jiang wrote:
>> CXL spec r3.1 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.
>>
>> Co-developed-by: Shiju Jose <shiju.jose@huawei.com>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>
>> v2:
>> - fix feature entry pointer math.
>> - free mbox_out in cxl_get_supported_features(). (Shiju, Jonathan)
>> - replace sizeof(struct) with feat_size. (Shiju)
>> - replace reserved in feature structs with u8 type. (Shiju)
>> - change cxl_feat_entry->effects to set_effects. (Shiju)
>> - rearrange assigment of cxl_mbox->entries. (Jonathan)
>> - Separate no features from actual error. (Jonathan)
>> - Fix missing kdoc for entries. (Jonathan)
>> ---
>>  drivers/cxl/core/mbox.c      | 178 +++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h         |  31 ++++++
>>  drivers/cxl/pci.c            |   4 +
>>  include/cxl/mailbox.h        |   4 +
>>  include/uapi/linux/cxl_mem.h |   1 +
>>  5 files changed, 218 insertions(+)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>> 880ac1dba3cc..4ba56f3d5a65 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -67,6 +67,7 @@ static struct cxl_mem_command
>cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
>> +	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD,
>0),
>
>Since this patch is only about internal usage then no need for a
>cxl_mem_commands entry which is the translation from CXL mailbox op-codes
>to user CXL commands ids.
>
>I think ideally CXL features ABI is only ever a fwctl concern. Lets try to see if we
>can get away with never defining a CXL user command code for them.
>
>>  };
>>
>>  /*
>> @@ -790,6 +791,183 @@ static const uuid_t log_uuid[] = {
>>  	[VENDOR_DEBUG_UUID] = DEFINE_CXL_VENDOR_DEBUG_UUID,  };
>>
>> +static void cxl_free_features(void *features) {
>> +	kvfree(features);
>> +}
>> +
>> +static int cxl_get_supported_features_count(struct cxl_dev_state
>> +*cxlds) {
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	struct cxl_mbox_get_sup_feats_out mbox_out;
>> +	struct cxl_mbox_get_sup_feats_in mbox_in;
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	int rc;
>> +
>> +	memset(&mbox_in, 0, sizeof(mbox_in));
>> +	mbox_in.count = cpu_to_le32(sizeof(mbox_out));
>> +	memset(&mbox_out, 0, sizeof(mbox_out));
>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +		.size_in = sizeof(mbox_in),
>> +		.payload_in = &mbox_in,
>> +		.size_out = sizeof(mbox_out),
>> +		.payload_out = &mbox_out,
>> +		.min_out = sizeof(mbox_out),
>> +	};
>> +	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	cxl_mbox->num_features = le16_to_cpu(mbox_out.supported_feats);
>
>I think this function should return a positive number of features or a negative
>error code, more below...

I  added this change in the EDAC CXL features setup and tested fine.

>
>> +
>> +	return 0;
>> +}
>> +
>> +int cxl_get_supported_features(struct cxl_dev_state *cxlds) {
>> +	struct cxl_mbox_get_sup_feats_out *mbox_out __free(kvfree) = NULL;
>> +	int remain_feats, max_size, max_feats, start, rc;
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	int feat_size = sizeof(struct cxl_feat_entry);
>> +	struct cxl_mbox_get_sup_feats_in mbox_in;
>> +	int hdr_size = sizeof(*mbox_out);
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	struct cxl_mem_command *cmd;
>> +	struct cxl_feat_entry *entry;
>> +
>> +	/* Get supported features is optional, need to check */
>> +	cmd =
>cxl_mem_find_command(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
>> +	if (!cmd)
>> +		return -EOPNOTSUPP;
>> +	if (!test_bit(cmd->info.id, cxl_mbox->enabled_cmds))
>> +		return -EOPNOTSUPP;
>> +
>> +	rc = cxl_get_supported_features_count(cxlds);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (!cxl_mbox->num_features) {
>> +		dev_dbg(cxl_mbox->host, "No CXL features enumerated.\n");
>> +		return 0;
>> +	}
>> +
>> +	struct cxl_feat_entry *entries __free(kvfree) =
>> +		kvmalloc(cxl_mbox->num_features * feat_size, GFP_KERNEL);
>> +
>> +	if (!entries)
>> +		return -ENOMEM;
>> +
>> +	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[0];
>> +
>> +	mbox_out = kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
>> +	if (!mbox_out)
>> +		return -ENOMEM;
>> +
>> +	start = 0;
>> +	remain_feats = cxl_mbox->num_features;
>> +	do {
>[..]
>> +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>> +		if (rc < 0)
>> +			return rc;
>
>Hmm, why "return rc" here, but "goto err" below?
>
>> +		if (mbox_cmd.size_out <= hdr_size) {
>> +			rc = -ENXIO;
>> +			goto err;
>
>So one of my new code smells for "needs more care" is cases where scoped-
>based-cleanup functions are still mixed with usage of "goto".
>
>In this case there are two dependent things to cleanup on error, "reset
>cxl_mbox->num_features" and "free @entries". So, to eliminate the goto, how
>about eliminate one of the things to cleanup with something like
>this:
>
>	struct cxl_features {
>		int num_features;
>		struct cxl_feat_entry entries[] __counted_by(num_features);
>	};
>
>	count = cxl_get_supported_features_count(cxl_mbox);
>	if (count <= 0)
>		return ...
>
>	struct cxl_features *features = __free(kvfree) =
>	        kvmalloc(struct_size(features, entries, count), GFP_KERNEL);
>
>	features->num_features = count;
>
>...and then use "cxl_mbox->features == NULL" as your "no features" case.

I  tried these changes in the EDAC CXL features setup and tested fine.

>
>
>> +		}
>> +
>> +		/*
>> +		 * Make sure retrieved out buffer is multiple of feature
>> +		 * entries.
>> +		 */
>> +		retrieved = mbox_cmd.size_out - hdr_size;
>> +		if (retrieved % feat_size) {
>> +			rc = -ENXIO;
>> +			goto err;
>> +		}
>> +
>> +		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) {
>> +			rc = -ENXIO;
>> +			goto err;
>> +		}
>> +
>> +		memcpy(entry, mbox_out->ents, retrieved);
>> +		entry++;
>> +		/*
>> +		 * 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);
>> +
>> +	cxl_mbox->entries = no_free_ptr(entries);
>> +	rc = devm_add_action_or_reset(cxl_mbox->host, cxl_free_features,
>> +				      cxl_mbox->entries);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +
>> +err:
>> +	cxl_mbox->num_features = 0;
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
>> +
>> +int cxl_get_supported_feature_entry(struct cxl_dev_state *cxlds, const uuid_t
>*feat_uuid,
>> +				    struct cxl_feat_entry *feat_entry_out) {
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	struct cxl_feat_entry *feat_entry;
>> +	int count;
>> +
>> +	/* Check CXL dev supports the feature */
>> +	feat_entry = &cxl_mbox->entries[0];
>> +	for (count = 0; count < cxl_mbox->num_features; count++, feat_entry++)
>{
>> +		if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
>> +			memcpy(feat_entry_out, feat_entry,
>sizeof(*feat_entry_out));
>
>Why copy-out? Would it not be sufficient to just return the index in the
>features->entries[] array?

I  added this change in the EDAC CXL features setup and tested fine.

>
>[..]
>> diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h index
>> cc894f07a435..03c4b8ad84c1 100644
>> --- a/include/cxl/mailbox.h
>> +++ b/include/cxl/mailbox.h
>> @@ -50,6 +50,8 @@ struct cxl_mbox_cmd {
>>   *                (CXL 3.1 8.2.8.4.3 Mailbox Capabilities Register)
>>   * @mbox_mutex: mutex protects device mailbox and firmware
>>   * @mbox_wait: rcuwait for mailbox
>> + * @num_features: number of supported features entries
>> + * @features: list of supported feature entries
>>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>>   */
>>  struct cxl_mailbox {
>> @@ -59,6 +61,8 @@ struct cxl_mailbox {
>>  	size_t payload_size;
>>  	struct mutex mbox_mutex; /* lock to protect mailbox context */
>>  	struct rcuwait mbox_wait;
>> +	int num_features;
>> +	struct cxl_feat_entry *entries;
>>  	int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd
>> *cmd);  };
>>
>> diff --git a/include/uapi/linux/cxl_mem.h
>> b/include/uapi/linux/cxl_mem.h index c6c0fe27495d..bd2535962f70 100644
>> --- a/include/uapi/linux/cxl_mem.h
>> +++ b/include/uapi/linux/cxl_mem.h
>> @@ -50,6 +50,7 @@
>>  	___C(GET_LOG_CAPS, "Get Log Capabilities"),			  \
>>  	___C(CLEAR_LOG, "Clear Log"),					  \
>>  	___C(GET_SUP_LOG_SUBLIST, "Get Supported Logs Sub-List"),	  \
>> +	___C(GET_SUPPORTED_FEATURES, "Get Supported Features"),
>	  \
>
>Again, for a patch that claims to be kernel internal only enabling, lets keep it
>internal-only.

Thanks,
Shiju