Re: [PATCH v5 07/15] cxl: Add FWCTL support to CXL

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

On Tue, 11 Feb 2025 11:28: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. The
> CXL PCI probe function will call cxl_setup_fwctl() after the cxl_memdev
> has been enumerated in order to setup FWCTL char device under the
> cxl_memdev like the existing memdev char device for issuing CXL raw
> mailbox commands from userspace via ioctls.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

A comment inline, but not a request to change anything for this
series.  More a suspicion that the simplification Dan suggested
will bite later :)

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 02558f46883d..db48a8fbde81 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -1,5 +1,6 @@

> +
> +int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)

I slightly wonder if we would be better passing
struct device *parent, struct cxl_dev_state *cxlds.

However, that can happen if this gets reuse I guess
rather than now.  We might have to put back a specific
fwctl structure to enable that given we could know
where to find cxlds in the structure containing the parent
device though.

Meh, might be my problem if I try to reuse this for
switch-cci which is my plan if I get time to do an RFC / PoC.



> +{
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_features_state *cxlfs;
> +	int rc;
> +
> +	cxlfs = to_cxlfs(cxlds);
> +	if (!cxlfs)
> +		return -ENODEV;
> +
> +	/* No need to setup FWCTL if there are no user allowed features found */
> +	if (!cxlfs->entries->num_user_features)
> +		return -ENODEV;
> +
> +	struct fwctl_device *fwctl_dev __free(free_fwctl_dev) =
> +		_fwctl_alloc_device(&cxlmd->dev, &cxlctl_ops, sizeof(*fwctl_dev));
> +	if (!fwctl_dev)
> +		return -ENOMEM;
> +
> +	rc = fwctl_register(fwctl_dev);
> +	if (rc)
> +		return rc;
> +
> +	cxlfs->fwctl_dev = fwctl_dev;
> +
> +	return devm_add_action_or_reset(&cxlmd->dev, free_memdev_fwctl,
> +					no_free_ptr(fwctl_dev));
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fwctl, "CXL");
> +
> +MODULE_IMPORT_NS("FWCTL");