Re: [RFC PATCH 08/13] fwctl/cxl: Add driver for CXL mailbox for handling CXL features commands

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

On Thu, 18 Jul 2024 14:32:26 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add an fwctl (auxiliary bus) driver to allow sending of CXL feature
> commands from userspace through as ioctls. Create a driver skeleton for
> initial setup.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Main question is why auxiliary bus?
We already do similar stuff in CXL on the cxl bus (e.g.
the CPMU devices etc) that then register a class
device with another subsystem.

>  GALAXYCORE GC0308 CAMERA SENSOR DRIVER
>  M:	Sebastian Reichel <sre@kernel.org>
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 9f0fe698414d..0f6ec85ef9c4 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -13,6 +13,8 @@
>  
>  static DECLARE_RWSEM(cxl_memdev_rwsem);
>  
> +#define CXL_ADEV_NAME "fwctl-cxl"
> +
>  /*
>   * An entire PCI topology full of devices should be enough for any
>   * config
> @@ -1030,6 +1032,7 @@ static const struct file_operations cxl_memdev_fops = {
>  struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>  				       struct cxl_dev_state *cxlds)
>  {
> +	struct auxiliary_device *adev;

Why an auxdev? It can be any convienient dev to which a driver
will bind. Why not spin one on the CXL bus for this purpose?

Maybe that creates and implied relationship you don't want, but
we already spin lots of devices there like the pmus, so not sure
why one more is a problem.

>  	struct cxl_memdev *cxlmd;
>  	struct device *dev;
>  	struct cdev *cdev;
> @@ -1056,11 +1059,27 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>  	if (rc)
>  		goto err;
>  
> +	adev = &cxlds->cxl_mbox.adev;
> +	adev->id = cxlmd->id;
> +	adev->name = CXL_ADEV_NAME;
> +	adev->dev.parent = dev;
> +
> +	rc = auxiliary_device_init(adev);
> +	if (rc)
> +		goto err;
> +
> +	rc = auxiliary_device_add(adev);
> +	if (rc)
> +		goto aux_err;
> +
>  	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);
>  	return cxlmd;
>  
> +aux_err:
> +	auxiliary_device_uninit(adev);
> +
>  err:
>  	/*
>  	 * The cdev was briefly live, shutdown any ioctl operations that

> diff --git a/drivers/fwctl/cxl/cxl.c b/drivers/fwctl/cxl/cxl.c
> new file mode 100644
> index 000000000000..518ba2afada8
> --- /dev/null
> +++ b/drivers/fwctl/cxl/cxl.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Intel Corporation
> + */
> +#include <linux/fwctl.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/cxl/mailbox.h>
> +#include <linux/auxiliary_bus.h>

Pick a standard ordering for header.
Any ordering is fine.

> +
> +static void cxlctl_remove(struct auxiliary_device *adev)
> +{
> +	struct cxlctl_dev *ctldev __free(cxlctl) = auxiliary_get_drvdata(adev);
> +
> +	fwctl_unregister(&ctldev->fwctl);

Same question on reference counting I asked in the set Jason posted
I don't get why we need the __free()

> +}
> +
> +static const struct auxiliary_device_id cxlctl_id_table[] = {
> +	{ .name = "CXL.fwctl", },
> +	{},
No trailing ,

> +};
> +MODULE_DEVICE_TABLE(auxiliary, cxlctl_id_table);
> +
> +static struct auxiliary_driver cxlctl_driver = {
> +	.name = "cxl_fwctl",
> +	.probe = cxlctl_probe,
> +	.remove = cxlctl_remove,
> +	.id_table = cxlctl_id_table,
> +};
> +
> +module_auxiliary_driver(cxlctl_driver);
> +
> +MODULE_IMPORT_NS(CXL);
> +MODULE_IMPORT_NS(FWCTL);
> +MODULE_DESCRIPTION("CXL fwctl driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");