Re: [PATCH v2 08/16] cxl: Add FWCTL support to the CXL memdev driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 31 Jan 2025 17:42:01 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add fwctl support code to allow sending of CXL feature commands from
> userspace through as ioctls via FWCTL. Provide initial setup bits.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Associate FWCTL with the 'cxl_memdev' through the memdev driver. (Dan)
> - I'm leaving Jonathan's tag off to get this looked at again.
Wise move ;)  Comments inline.  Mind you I'm rereading the lot
as been off a few days and this is all gone from my head!



> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 82598a409acc..2dcfefdfa6b3 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c

>  
> +/* The __weak symbol gets replaced by FWCTL allocation function in fwctl.c */
> +struct cxl_features_state * __weak devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +{
> +	struct device *dev = &cxlmd->dev;
> +	struct cxl_features_state *cxlfs =
> +		devm_kzalloc(dev, sizeof(*cxlfs), GFP_KERNEL);
> +
> +	if (!cxlfs)
> +		return ERR_PTR(-ENOMEM);
This is returning an error pointer, but the DEFINE FREE assumes
NULL I think for the failed to allocate case. Or just return NULL
and don't modify that.

> +
> +	cxlfs->cxlmd = cxlmd;
> +
> +	return cxlfs;
> +}
> +
> +/* The __weak symbol gets replaced by FWCTL free function in fwctl.c */
> +void __weak devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +{
> +	devm_kfree(&cxlmd->dev, cxlmd->cxlfs);
> +	cxlmd->cxlfs = NULL;

This missing clear of hanging pointer I mentioned back in earlier patch.

> +}
> +
>  static void cxl_cxlfs_free(struct cxl_features_state *cxlfs)
>  {
> -	struct device *dev = &cxlfs->cxlmd->dev;
> -
> -	devm_kfree(dev, cxlfs);
> +	devm_cxlfs_free(cxlfs->cxlmd);
>  }
>  
>  DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(_T))
> @@ -185,15 +208,13 @@ DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(_T))

As above, needs to be if (!IS_ERR_OR_NULL(_T)) I think.

>   */
>  int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>  {
> -	struct device *dev = &cxlmd->dev;
>  	int rc;
>  
>  	struct cxl_features_state *cxlfs __free(free_cxlfs) =
> -		devm_kzalloc(dev, sizeof(*cxlfs), GFP_KERNEL);
> +		devm_cxlfs_allocate(cxlmd);

As per early comments. I'd drag this much earlier in the series
though make only add the __weak markings and comments in this
patch.  That will reduce churn.

>  	if (!cxlfs)
>  		return -ENOMEM;
>  
> -	cxlfs->cxlmd = cxlmd;
>  	enumerate_feature_cmds(cxlmd, cxlfs);
>  	rc = get_supported_features(cxlmd, cxlfs);
>  	if (rc)
> @@ -204,3 +225,5 @@ int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
> +
> +MODULE_IMPORT_NS("CXL");

> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> new file mode 100644
> index 000000000000..d8fd68ac8982
> --- /dev/null
> +++ b/drivers/cxl/fwctl.c
> @@ -0,0 +1,88 @@

> +static void remove_cxlfs(void *_cxlfs)
> +{
> +	struct cxl_features_state *cxlfs = _cxlfs;
> +	struct cxl_memdev *cxlmd = cxlfs->cxlmd;
> +	struct fwctl_device *fwctl = &cxlfs->fwctl;
> +
> +	cxlmd->cxlfs = NULL;

This is a little nasty.  Originally set in enumerate commands, so not
obvious it should be unwound down in here. Perhaps a comment is
enough.

> +	fwctl_unregister(fwctl);
> +	fwctl_put(fwctl);
> +}
> +
> +DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) fwctl_put(&_T->fwctl))
> +
> +static struct cxl_features_state *
> +__devm_cxl_cfs_allocate(struct cxl_memdev *cxlmd, const struct fwctl_ops *ops)
> +{
> +	struct device *dev = &cxlmd->dev;
> +	int rc;
> +
> +	struct cxl_features_state *cxlfs __free(free_cxlfs) =
> +		fwctl_alloc_device(dev, ops, struct cxl_features_state, fwctl);
> +	if (!cxlfs)
> +		return ERR_PTR(-ENOMEM);

As above, the caller of this is used with __free that is assuming NULL
for failed.

> +
> +	cxlfs->cxlmd = cxlmd;
> +	rc = fwctl_register(&cxlfs->fwctl);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	rc = devm_add_action_or_reset(dev, remove_cxlfs, cxlfs);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return no_free_ptr(cxlfs);
> +}

This use of weak functions is a bit too clever for me to be entirely happy but it
seems right so fair enough.

> +
> +struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +{
> +	return __devm_cxl_cfs_allocate(cxlmd, &cxlctl_ops);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxlfs_allocate, "CXL");
> +
> +void devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +{
> +	devm_release_action(&cxlmd->dev, remove_cxlfs, cxlmd);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxlfs_free, "CXL");
> +
> +MODULE_IMPORT_NS("FWCTL");