| Message ID | 20240718213446.1750135-9-dave.jiang@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fwctl/cxl: Add CXL feature commands support via fwctl | expand |
On Thu, Jul 18, 2024 at 02:32:26PM -0700, Dave Jiang wrote: > 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" > + This should be in a header? > @@ -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; It goes there > +static const struct auxiliary_device_id cxlctl_id_table[] = { > + { .name = "CXL.fwctl", }, > + {}, But then how does this match work? Shouldn't it be CXL_ADEV_NAME ? Jason
On 7/22/24 8:31 AM, Jason Gunthorpe wrote: > On Thu, Jul 18, 2024 at 02:32:26PM -0700, Dave Jiang wrote: > >> 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" >> + > > This should be in a header? yes. will fix. > >> @@ -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; > > It goes there > >> +static const struct auxiliary_device_id cxlctl_id_table[] = { >> + { .name = "CXL.fwctl", }, >> + {}, > > But then how does this match work? Shouldn't it be CXL_ADEV_NAME ? Yes will fix. > > Jason
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");
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");
On Tue, Sep 24, 2024 at 04:44:19PM -0700, Dave Jiang wrote: > >> @@ -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? IMHO bus members should adhere to some kinds of rules about the bus itself. Devices should have the same set of paramters/etc. I don't know what cxl_bus is, but I'd be surprised that something with that name has SW only entities on it as devices?? aux bus was specifically for SW only entities that are just for linking software components. Jason
On 9/26/24 10:37 AM, Jason Gunthorpe wrote: > On Tue, Sep 24, 2024 at 04:44:19PM -0700, Dave Jiang wrote: > >>>> @@ -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? > > IMHO bus members should adhere to some kinds of rules about the bus > itself. Devices should have the same set of paramters/etc. I don't > know what cxl_bus is, but I'd be surprised that something with that > name has SW only entities on it as devices?? So I did do the conversion yesterday and it ended up looking something like this [1]. cxl_bus has cxl objects that are hardware representations (endpoints, ports, decoders, pmu, etc) or software representations (regions). So it's probably ok to keep all the cxl stuff on the same bus. I don't have a strong feeling on whether use cxl bus that already exists or using aux bus. > > aux bus was specifically for SW only entities that are just for > linking software components. > > Jason [1]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?h=cxl/fwctl&id=231da137a959331cc5bc80ef4acb3d7e667bfbad
diff --git a/MAINTAINERS b/MAINTAINERS index 3e1934b7e044..5d6c470c06ea 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9073,6 +9073,13 @@ L: linux-kernel@vger.kernel.org S: Maintained F: drivers/fwctl/mlx5/ +FWCTL CXL DRIVER +M: Dave Jiang <dave.jiang@intel.com> +R: Dan Williams <dan.j.williams@intel.com> +L: linux-cxl@vger.kernel.org +S: Maintained +F: drivers/fwctl/cxl/ + 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; 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/Kconfig b/drivers/fwctl/Kconfig index e5ee2d46d431..e49903a9d0d3 100644 --- a/drivers/fwctl/Kconfig +++ b/drivers/fwctl/Kconfig @@ -19,5 +19,14 @@ config FWCTL_MLX5 This will allow configuration and debug tools to work out of the box on mainstream kernel. + If you don't know what to do here, say N. + +config FWCTL_CXL + tristate "CXL fwctl driver" + depends on CXL_BUS + help + CXLCTL provides interface for the user process to access user allowed + mailbox commands for CXL device. + If you don't know what to do here, say N. endif diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile index 1c535f694d7f..bd356e6f2e5a 100644 --- a/drivers/fwctl/Makefile +++ b/drivers/fwctl/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_FWCTL) += fwctl.o obj-$(CONFIG_FWCTL_MLX5) += mlx5/ +obj-$(CONFIG_FWCTL_CXL) += cxl/ fwctl-y += main.o diff --git a/drivers/fwctl/cxl/Makefile b/drivers/fwctl/cxl/Makefile new file mode 100644 index 000000000000..623194521572 --- /dev/null +++ b/drivers/fwctl/cxl/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_FWCTL_CXL) += cxl_fwctl.o + +cxl_fwctl-y += cxl.o 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> + +struct cxlctl_uctx { + struct fwctl_uctx uctx; + u32 uctx_caps; + u32 uctx_uid; +}; + +struct cxlctl_dev { + struct fwctl_device fwctl; + struct cxl_mailbox *mbox; +}; + +DEFINE_FREE(cxlctl, struct cxlctl_dev *, if (_T) fwctl_put(&_T->fwctl)) + +static int cxlctl_open_uctx(struct fwctl_uctx *uctx) +{ + return 0; +} + +static void cxlctl_close_uctx(struct fwctl_uctx *uctx) +{ +} + +static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length) +{ + /* Place holder */ + return ERR_PTR(-EOPNOTSUPP); +} + +static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, + void *rpc_in, size_t in_len, size_t *out_len) +{ + /* Place holder */ + return ERR_PTR(-EOPNOTSUPP); +} + +static const struct fwctl_ops cxlctl_ops = { + .device_type = FWCTL_DEVICE_TYPE_CXL, + .uctx_size = sizeof(struct cxlctl_uctx), + .open_uctx = cxlctl_open_uctx, + .close_uctx = cxlctl_close_uctx, + .info = cxlctl_info, + .fw_rpc = cxlctl_fw_rpc, +}; + +static int cxlctl_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) +{ + struct cxl_mailbox *mbox = container_of(adev, struct cxl_mailbox, adev); + struct cxlctl_dev *cxlctl __free(cxlctl) = + fwctl_alloc_device(mbox->host, &cxlctl_ops, + struct cxlctl_dev, fwctl); + int rc; + + if (!cxlctl) + return -ENOMEM; + + cxlctl->mbox = mbox; + + rc = fwctl_register(&cxlctl->fwctl); + if (rc) + return rc; + + auxiliary_set_drvdata(adev, no_free_ptr(cxlctl)); + + return 0; +} + +static void cxlctl_remove(struct auxiliary_device *adev) +{ + struct cxlctl_dev *ctldev __free(cxlctl) = auxiliary_get_drvdata(adev); + + fwctl_unregister(&ctldev->fwctl); +} + +static const struct auxiliary_device_id cxlctl_id_table[] = { + { .name = "CXL.fwctl", }, + {}, +}; +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"); diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h index 49a357e1bc71..6edcc38a6eee 100644 --- a/include/uapi/fwctl/fwctl.h +++ b/include/uapi/fwctl/fwctl.h @@ -43,6 +43,7 @@ enum { enum fwctl_device_type { FWCTL_DEVICE_TYPE_ERROR = 0, FWCTL_DEVICE_TYPE_MLX5 = 1, + FWCTL_DEVICE_TYPE_CXL = 2, }; /**
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> --- MAINTAINERS | 7 +++ drivers/cxl/core/memdev.c | 19 +++++++ drivers/fwctl/Kconfig | 9 ++++ drivers/fwctl/Makefile | 1 + drivers/fwctl/cxl/Makefile | 4 ++ drivers/fwctl/cxl/cxl.c | 103 +++++++++++++++++++++++++++++++++++++ include/uapi/fwctl/fwctl.h | 1 + 7 files changed, 144 insertions(+) create mode 100644 drivers/fwctl/cxl/Makefile create mode 100644 drivers/fwctl/cxl/cxl.c