Re: [PATCH v2 02/16] cxl: Enumerate feature commands

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

On 2/3/25 5:06 AM, Jonathan Cameron wrote:
> On Fri, 31 Jan 2025 17:41:55 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add feature commands enumeration code in order to detect and enumerate
>> the 3 feature related commands "get supported features", "get feature",
>> and "set feature". The enumeration will help determine whether the driver
>> can issue any of the 3 commands to the device.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v2:
>> - Setup allocation of cxlfs and move enumeration of commands to
>>   the setup function.
>> - Introduce features_capability enum. (Dan)
>> - Name the setup function devm_cxl_add_features(). (Dan)
>> - Drop 'cxl_mem_command' support. (Dan)
>> - Move feature support to mem probe. (Dan)
>> - Remove comma at last entry of data structs. (Jonathan)
> Got carried away on this one.   Only drop them for termination
> entries.  So MAX or NULL or similar.
> 
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index bdb8f060f2c1..25fb7bd770b3 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -69,6 +69,29 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
>>  };
>>  
>> +static u16 cxl_feature_commands[] = {
>> +	CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +	CXL_MBOX_OP_GET_FEATURE,
>> +	CXL_MBOX_OP_SET_FEATURE
> 
> This one does want a trailing comma. Might be more feature commands in future.

ok

> 
>> +};
> 
>> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
>> new file mode 100644
>> index 000000000000..13b0b29ee102
>> --- /dev/null
>> +++ b/drivers/cxl/features.c
>> @@ -0,0 +1,55 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
>> +#include <linux/device.h>
>> +#include <cxl/mailbox.h>
>> +#include <cxl/features.h>
>> +#include "cxl.h"
>> +#include "cxlmem.h"
>> +#include "features.h"
>> +
>> +static void enumerate_feature_cmds(struct cxl_memdev *cxlmd)
>> +{
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>> +	struct cxl_features_state *cxlfs = cxlds->cxlmd->cxlfs;
> Going in circles?
> 	cxlmd->cxlfs;

yeah oops. should be cleaned up with the other changes going on in this patch

> 
> 
>> +	int fid;
>> +
>> +	fid = cxl_get_feature_command_id(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
>> +	if (!test_bit(fid, cxl_mbox->feature_cmds))
>> +		return;
>> +
>> +	fid = cxl_get_feature_command_id(CXL_MBOX_OP_GET_FEATURE);
>> +	if (!test_bit(fid, cxl_mbox->feature_cmds))
>> +		return;
>> +
>> +	cxlfs->cap = CXL_FEATURES_RO;
>> +
>> +	fid = cxl_get_feature_command_id(CXL_MBOX_OP_SET_FEATURE);
>> +	if (!test_bit(fid, cxl_mbox->feature_cmds))
>> +		return;
>> +
>> +	cxlfs->cap = CXL_FEATURES_RW;
>> +}
>> +
>> +/**
>> + * devm_cxl_add_features() - Allocate and initialize features context
>> + * @cxlmd: CXL memory device
>> + *
>> + * Return 0 on success or -errno on failure.
>> + */
>> +int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>> +{
>> +	struct cxl_features_state *cxlfs;
>> +	struct device *dev = &cxlmd->dev;
>> +
>> +	cxlfs = devm_kzalloc(dev, sizeof(*cxlfs), GFP_KERNEL);
>> +	if (!cxlfs)
>> +		return -ENOMEM;
>> +
>> +	cxlmd->cxlfs = cxlfs;
> 
> So I was curious about this one and why we leave the pointer
> hanging in the remove path + whether the two way pointer nest
> is needed.
> 
> That gets dealt with in patch 8.
> 
> I'd be tempted to drag the bones of dev_cxlfs_allocate() to this
> patch so that you can do that cleanup now and avoid some of the churn
> of the later patches.

ok I'll do that. 

> 
> As for the two way pointer nest. I'm not sure it is needed.
> Seems like most if not all places we use cxlmd->cxlfs have
> just gotten it in the caller from cxlfs->cxlmd.
> e.g. cxl_cxlfs_free() in patch 8. For here we could
> just pass it into enumerate_feature_cmds.
> 
> Maybe Shiju's set needs it. I haven't checked.

I think he does with calling of cxl_get_supported_feature_entry(). Need a way to go from cxlmd to cxlfs.
> 
> 
> 
> 
>> +	cxlfs->cxlmd = cxlmd;
>> +	enumerate_feature_cmds(cxlmd);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
> 
>> diff --git a/include/cxl/features.h b/include/cxl/features.h
>> new file mode 100644
>> index 000000000000..eadf0d56553f
>> --- /dev/null
>> +++ b/include/cxl/features.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright(c) 2024-2025 Intel Corporation. */
>> +#ifndef __CXL_FEATURES_H__
>> +#define __CXL_FEATURES_H__
>> +
>> +struct cxl_mailbox;
>> +
>> +/* Index IDs for CXL mailbox Feature commands */
>> +enum feature_cmds {
>> +	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
>> +	CXL_FEATURE_ID_GET_FEATURE,
>> +	CXL_FEATURE_ID_SET_FEATURE,
>> +	CXL_FEATURE_ID_MAX
>> +};
>> +
>> +/* Feature commands capability supported by a device */
>> +enum cxl_features_capability {
>> +	CXL_FEATURES_NONE = 0,
>> +	CXL_FEATURES_RO,
>> +	CXL_FEATURES_RW
> 
> Trivial but this one should have a trailing comma.  Maybe
> we'll have a write only one!  Anyhow, it's not a terminating
> entry so we shouldn't assume nothing comes after it.

ok

> 
>> +};
>> +
>> +/**
>> + * struct cxl_features_state - The Features state for the device
>> + * @cxlmd: Pointer to cxl mem device
>> + * @cap: Feature commands capability
>> + * @num_features: total Features supported by the device
>> + */
>> +struct cxl_features_state {
>> +	struct cxl_memdev *cxlmd;
>> +	enum cxl_features_capability cap;
>> +	int num_features;
>> +};
>> +
>> +#endif
>