Re: [PATCH v3 03/16] 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.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Use struct_size() instead of open coding the calculation. (Jonathan)
> ---
>  drivers/cxl/features.c | 138 +++++++++++++++++++++++++++++++++++++++++
>  include/cxl/features.h |  57 +++++++++++++++++
>  2 files changed, 195 insertions(+)
> 
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 958e4828a58d..4d8c603121f0 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
> @@ -7,6 +7,139 @@
>  #include "cxlmem.h"
>  #include "features.h"
>  
> +static void cxl_free_feature_entries(void *entries)
> +{
> +	kvfree(entries);
> +}
> +
> +static int cxl_get_supported_features_count(struct cxl_mailbox *cxl_mbox)
> +{
[..]
> +
> +	cxlfs->num_features = count;
> +	cxlfs->entries = no_free_ptr(entries);
> +	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
> +					cxlfs->entries);

This is a memory leak. At this point cxlfs->entries is NULL having been
marked NULL by no_free_ptr().

It is awkward to mix a return code, a return buffer, and a return
features count all at the same time near a devm handoff. I would suggest
solving it by:

1/ skip calling devm_add_action_or_reset() inside of
   get_supported_features(), make the caller responsible for devm
   wrapping any allocations.

2/ Convey the error code and the object via an ERR_PTR() from
   get_supported_features()

3/ Move "num_features" into the allocation for @entries. Something like:

struct cxl_features_state {
     struct cxl_memdev *cxlmd;
     enum cxl_features_capability cap;
     struct cxl_feat_entries {
         int num_features;
         struct cxl_feat_entry ent[] __counted_by(num_features);
     } *entries;
};

...and then have get_supported_features() return a 'struct
cxl_feat_entries *' pointer.

> +}
> +
>  static void enumerate_feature_cmds(struct cxl_memdev *cxlmd,
>  				   struct cxl_features_state *cxlfs)
>  {
> @@ -77,12 +210,17 @@ DEFINE_FREE(cxl_free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(
>   */
>  int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>  {
> +	int rc;
> +
>  	struct cxl_features_state *cxlfs __free(cxl_free_cxlfs) =
>  		devm_cxlfs_allocate(cxlmd);

devm and scope-based should never appear in the same statement.

>  	if (!cxlfs)
>  		return -ENOMEM;
>  
>  	enumerate_feature_cmds(cxlmd, cxlfs);
> +	rc = get_supported_features(cxlmd, cxlfs);
> +	if (rc)
> +		return rc;
>  
>  	cxlmd->cxlfs = no_free_ptr(cxlfs);
>  
> diff --git a/include/cxl/features.h b/include/cxl/features.h
> index 8e1830f0ccba..c65760342f97 100644
> --- a/include/cxl/features.h
> +++ b/include/cxl/features.h
> @@ -3,6 +3,8 @@
>  #ifndef __CXL_FEATURES_H__
>  #define __CXL_FEATURES_H__
>  
> +#include <linux/uuid.h>
> +
>  struct cxl_mailbox;
>  
>  /* Index IDs for CXL mailbox Feature commands */
> @@ -25,11 +27,66 @@ enum cxl_features_capability {
>   * @cxlmd: Pointer to cxl mem device
>   * @cap: Feature commands capability
>   * @num_features: total Features supported by the device
> + * @entries: Feature detail entries fetched from the device
>   */
>  struct cxl_features_state {
>  	struct cxl_memdev *cxlmd;
>  	enum cxl_features_capability cap;
>  	int num_features;
> +	struct cxl_feat_entry *entries;

Per above, this organization results in some buggy gymanistics.