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

[Dan Williams <dan.j.williams@intel.com>]

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.

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>