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

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

On 2/7/25 4:56 PM, Dan Williams wrote:
> 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>
> [..]
>> +/**
>> + * 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;
>> +
>> +	cxlds->cxlfs = cxlfs;
>> +	rc = devm_add_action_or_reset(cxlds->dev, free_cxlfs, no_free_ptr(cxlfs));
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
> 
> One small cleanup you can do here when applying is reduce these last 5
> lines to:
> 
>         return devm_add_action_or_reset(cxlds->dev, free_cxlfs,
>                                         no_free_ptr(cxlfs));
> 
> [..]
>> +/**
>> + * struct cxl_features_state - The Features state for the device
>> + * @cxlds: Pointer to CXL device state
>> + * @cap: Feature commands capability
>> + * @entries: CXl feature entry context
>> + *	@num_features: total Features supported by the device
>> + *	@ent: Flex array of Feature detail entries from the device
>> + */
>> +struct cxl_features_state {
>> +	struct cxl_dev_state *cxlds;
>> +	struct cxl_feat_entries {
>> +		int num_features;
>> +		struct cxl_feat_entry ent[] __counted_by(num_features);
>> +	} *entries;
>> +};
>> +
>> +#ifdef CONFIG_CXL_FEATURES
>> +inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds);
> 
> Shouldn't this be:
> 
> static inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds)
> {
> 	return cxlds->cxlfs;
> }
> 
> ...no need to do an out of line function for this.

So the reason it is an out of line function is to not expose 'struct cxl_dev_state' globally. Otherwise it needs to be in a public header in include/cxl/ to work.

DJ

> 
> Another small fixup you can do when applying.
> 
> Otherwise, this now all looks good to me:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>