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

[Dave Jiang <dave.jiang@intel.com>]

On 7/26/24 11:02 AM, Jonathan Cameron wrote:
> 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?

It was convenient at the time. I just copied what Jason had in the mlx5 driver.

> 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.

Sure. I'll adapt it to the cxl_bus. 

DJ

> 
>>  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");