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

Dave Jiang
2024-09-24T23:44:21Z

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