| Message ID | 1-v2-940e479ceba9+3821-fwctl_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce fwctl subystem | expand |
On Mon, Jun 24, 2024 at 07:47:25PM -0300, Jason Gunthorpe wrote: > +/** > + * fwctl_alloc_device - Allocate a fwctl > + * @parent: Physical device that provides the FW interface > + * @ops: Driver ops to register > + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device > + * @member: Name of the struct fwctl_device in @drv_struct > + * > + * This allocates and initializes the fwctl_device embedded in the drv_struct. > + * Upon success the pointer must be freed via fwctl_put(). Returns NULL on > + * failure. Returns a 'drv_struct *' on success, NULL on error. > + */ Sphinx reports htmldocs warning: Documentation/userspace-api/fwctl:195: ./include/linux/fwctl.h:72: WARNING: Inline emphasis start-string without end-string. I have to escape the pointer (while also cleaning up redundant wordings on error case): ---- >8 ---- diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h index 294cfbf63306a2..ddadbe15189b45 100644 --- a/include/linux/fwctl.h +++ b/include/linux/fwctl.h @@ -70,8 +70,8 @@ struct fwctl_device *_fwctl_alloc_device(struct device *parent, * @member: Name of the struct fwctl_device in @drv_struct * * This allocates and initializes the fwctl_device embedded in the drv_struct. - * Upon success the pointer must be freed via fwctl_put(). Returns NULL on - * failure. Returns a 'drv_struct *' on success, NULL on error. + * Upon success the pointer must be freed via fwctl_put(). Returns a + * 'drv_struct \*' on success, NULL on error. */ #define fwctl_alloc_device(parent, ops, drv_struct, member) \ container_of(_fwctl_alloc_device( \ Thanks.
On Tue, Jun 25, 2024 at 11:47:11AM +0700, Bagas Sanjaya wrote: > On Mon, Jun 24, 2024 at 07:47:25PM -0300, Jason Gunthorpe wrote: > > +/** > > + * fwctl_alloc_device - Allocate a fwctl > > + * @parent: Physical device that provides the FW interface > > + * @ops: Driver ops to register > > + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device > > + * @member: Name of the struct fwctl_device in @drv_struct > > + * > > + * This allocates and initializes the fwctl_device embedded in the drv_struct. > > + * Upon success the pointer must be freed via fwctl_put(). Returns NULL on > > + * failure. Returns a 'drv_struct *' on success, NULL on error. > > + */ > > Sphinx reports htmldocs warning: > > Documentation/userspace-api/fwctl:195: ./include/linux/fwctl.h:72: WARNING: Inline emphasis start-string without end-string. > > I have to escape the pointer (while also cleaning up redundant wordings on > error case): Got it thanks Jason
On Mon, 24 Jun 2024 19:47:25 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > Create the class, character device and functions for a fwctl driver to > un/register to the subsystem. > > A typical fwctl driver has a sysfs presence like: > > $ ls -l /dev/fwctl/fwctl0 > crw------- 1 root root 250, 0 Apr 25 19:16 /dev/fwctl/fwctl0 > > $ ls /sys/class/fwctl/fwctl0 > dev device power subsystem uevent > > $ ls /sys/class/fwctl/fwctl0/device/infiniband/ > ibp0s10f0 > > $ ls /sys/class/infiniband/ibp0s10f0/device/fwctl/ > fwctl0/ > > $ ls /sys/devices/pci0000:00/0000:00:0a.0/fwctl/fwctl0 > dev device power subsystem uevent > > Which allows userspace to link all the multi-subsystem driver components > together and learn the subsystem specific names for the device's > components. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Hi Jason, Mostly looking at this to get my head around what the details are, but whilst I'm reading might as well offer some review comments. I'm not a fan of too many mini patches as it makes it harder to review rather than easier, but meh, I know others prefer it this way. If you are going to do it though, comments need to be carefully tracking what they are talking about. Jonathan ... > diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c > new file mode 100644 > index 00000000000000..6e9bf15c743b5c > --- /dev/null > +++ b/drivers/fwctl/main.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES > + */ > +#define pr_fmt(fmt) "fwctl: " fmt > +#include <linux/fwctl.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/container_of.h> > +#include <linux/fs.h> Trivial: Pick an ordering scheme perhaps as then we know where you'd like new headers to be added. > + > +enum { > + FWCTL_MAX_DEVICES = 256, > +}; > +static dev_t fwctl_dev; > +static DEFINE_IDA(fwctl_ida); > +static struct fwctl_device * > +_alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size) > +{ > + struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL); > + int devnum; > + > + if (!fwctl) > + return NULL; I'd put a blank line here. > + fwctl->dev.class = &fwctl_class; > + fwctl->dev.parent = parent; > + > + devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL); > + if (devnum < 0) > + return NULL; > + fwctl->dev.devt = fwctl_dev + devnum; > + > + device_initialize(&fwctl->dev); > + return_ptr(fwctl); > +} > + > +/* Drivers use the fwctl_alloc_device() wrapper */ > +struct fwctl_device *_fwctl_alloc_device(struct device *parent, > + const struct fwctl_ops *ops, > + size_t size) > +{ > + struct fwctl_device *fwctl __free(fwctl) = > + _alloc_device(parent, ops, size); > + > + if (!fwctl) > + return NULL; > + > + cdev_init(&fwctl->cdev, &fwctl_fops); > + fwctl->cdev.owner = THIS_MODULE; Owned by fwctl core, not the parent driver? Perhaps a comment on why. I guess related to the lifetime being independent of parent driver. > + > + if (dev_set_name(&fwctl->dev, "fwctl%d", fwctl->dev.devt - fwctl_dev)) > + return NULL; > + > + fwctl->ops = ops; > + return_ptr(fwctl); > +} > +EXPORT_SYMBOL_NS_GPL(_fwctl_alloc_device, FWCTL); > + > +/** > + * fwctl_register - Register a new device to the subsystem > + * @fwctl: Previously allocated fwctl_device > + * > + * On return the device is visible through sysfs and /dev, driver ops may be > + * called. > + */ > +int fwctl_register(struct fwctl_device *fwctl) > +{ > + int ret; > + > + ret = cdev_device_add(&fwctl->cdev, &fwctl->dev); > + if (ret) > + return ret; > + return 0; Doesn't look like this ever gets more complex so return cdev_device_add(...) If you expect to see more here in near future maybe fair enough to keep the handling as is. > +} > +EXPORT_SYMBOL_NS_GPL(fwctl_register, FWCTL); > + > +/** > + * fwctl_unregister - Unregister a device from the subsystem > + * @fwctl: Previously allocated and registered fwctl_device > + * > + * Undoes fwctl_register(). On return no driver ops will be called. The > + * caller must still call fwctl_put() to free the fwctl. > + * > + * Unregister will return even if userspace still has file descriptors open. > + * This will call ops->close_uctx() on any open FDs and after return no driver > + * op will be called. The FDs remain open but all fops will return -ENODEV. Perhaps bring the docs in with the support? I got (briefly) confused by the lack of a path to close_uctx() in here. > + * > + * The design of fwctl allows this sort of disassociation of the driver from the > + * subsystem primarily by keeping memory allocations owned by the core subsytem. > + * The fwctl_device and fwctl_uctx can both be freed without requiring a driver > + * callback. This allows the module to remain unlocked while FDs are open. > + */ > +void fwctl_unregister(struct fwctl_device *fwctl) > +{ > + cdev_device_del(&fwctl->cdev, &fwctl->dev); > + > + /* > + * The driver module may unload after this returns, the op pointer will > + * not be valid. > + */ > + fwctl->ops = NULL; I'd bring that in with the logic doing close_uctx() etc as then it will align with the comments that I'd also suggest only adding there (patch 2 I think). > +} > +EXPORT_SYMBOL_NS_GPL(fwctl_unregister, FWCTL); > diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h > new file mode 100644 > index 00000000000000..ef4eaa87c945e4 > --- /dev/null > +++ b/include/linux/fwctl.h > @@ -0,0 +1,68 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES > + */ > +#ifndef __LINUX_FWCTL_H > +#define __LINUX_FWCTL_H > +#include <linux/device.h> > +#include <linux/cdev.h> > +#include <linux/cleanup.h> > + > +struct fwctl_device; > +struct fwctl_uctx; > + > +struct fwctl_ops { > +}; > + > +/** > + * struct fwctl_device - Per-driver registration struct > + * @dev: The sysfs (class/fwctl/fwctlXX) device > + * > + * Each driver instance will have one of these structs with the driver > + * private data following immeidately after. This struct is refcounted, immediately > + * it is freed by calling fwctl_put(). > + */ > +struct fwctl_device { > + struct device dev; > + /* private: */ > + struct cdev cdev; > + const struct fwctl_ops *ops; > +}; > + > +struct fwctl_device *_fwctl_alloc_device(struct device *parent, > + const struct fwctl_ops *ops, > + size_t size); > +/** > + * fwctl_alloc_device - Allocate a fwctl > + * @parent: Physical device that provides the FW interface > + * @ops: Driver ops to register > + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device > + * @member: Name of the struct fwctl_device in @drv_struct > + * > + * This allocates and initializes the fwctl_device embedded in the drv_struct. > + * Upon success the pointer must be freed via fwctl_put(). Returns NULL on > + * failure. Returns a 'drv_struct *' on success, NULL on error. > + */ > +#define fwctl_alloc_device(parent, ops, drv_struct, member) \ > + container_of(_fwctl_alloc_device( \ > + parent, ops, \ > + sizeof(drv_struct) + \ > + BUILD_BUG_ON_ZERO( \ > + offsetof(drv_struct, member))), \ Doesn't that fire a build_bug when the member is at the start of drv_struct? Or do I have that backwards? Does container_of() safely handle a NULL? I'm staring at the definition and can't spot code to do that in 6.10 > + drv_struct, member) > +
On Fri, Jul 26, 2024 at 03:30:42PM +0100, Jonathan Cameron wrote: > Mostly looking at this to get my head around what the details are, > but whilst I'm reading might as well offer some review comments. Thanks! > I'm not a fan of too many mini patches as it makes it harder > to review rather than easier, but meh, I know others prefer > it this way. If you are going to do it though, comments > need to be carefully tracking what they are talking about. Yeah, I don't like it so much either, but given the debate on this series I structured it so you can read the commit messages only and have a pretty good idea what is inside. > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES > > + */ > > +#define pr_fmt(fmt) "fwctl: " fmt > > +#include <linux/fwctl.h> > > +#include <linux/module.h> > > +#include <linux/slab.h> > > +#include <linux/container_of.h> > > +#include <linux/fs.h> > > Trivial: Pick an ordering scheme perhaps as then we know where you'd > like new headers to be added. Heh, I think it is random ordered :) But sure lets sort by name, though linux/fwctl.h does go first. Putting headers first in at least one c file is a neat trick to ensure they self-compile and don't miss their own #includess #define pr_fmt(fmt) "fwctl: " fmt #include <linux/fwctl.h> #include <linux/container_of.h> #include <linux/fs.h> #include <linux/module.h> #include <linux/slab.h> > > +static struct fwctl_device * > > +_alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size) > > +{ > > + struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL); > > + int devnum; > > + > > + if (!fwctl) > > + return NULL; > > I'd put a blank line here. Done > > +/* Drivers use the fwctl_alloc_device() wrapper */ > > +struct fwctl_device *_fwctl_alloc_device(struct device *parent, > > + const struct fwctl_ops *ops, > > + size_t size) > > +{ > > + struct fwctl_device *fwctl __free(fwctl) = > > + _alloc_device(parent, ops, size); > > + > > + if (!fwctl) > > + return NULL; > > + > > + cdev_init(&fwctl->cdev, &fwctl_fops); > > + fwctl->cdev.owner = THIS_MODULE; > > Owned by fwctl core, not the parent driver? Perhaps a comment on why. > I guess related to the lifetime being independent of parent driver. Yes. /* * The driver module is protected by fwctl_register/unregister(), * unregister won't complete until we are done with the driver's module. */ fwctl->cdev.owner = THIS_MODULE; > > +int fwctl_register(struct fwctl_device *fwctl) > > +{ > > + int ret; > > + > > + ret = cdev_device_add(&fwctl->cdev, &fwctl->dev); > > + if (ret) > > + return ret; > > + return 0; > > Doesn't look like this ever gets more complex so > > return cdev_device_add(...) > > If you expect to see more here in near future maybe fair enough > to keep the handling as is. Sure, I was expecting more when I wrote it then it turned out there wasn't > > + * fwctl_unregister - Unregister a device from the subsystem > > + * @fwctl: Previously allocated and registered fwctl_device > > + * > > + * Undoes fwctl_register(). On return no driver ops will be called. The > > + * caller must still call fwctl_put() to free the fwctl. > > + * > > + * Unregister will return even if userspace still has file descriptors open. > > + * This will call ops->close_uctx() on any open FDs and after return no driver > > + * op will be called. The FDs remain open but all fops will return -ENODEV. > > Perhaps bring the docs in with the support? I got (briefly) confused > by the lack of a path to close_uctx() in here. Okay, that paragraph can be shifted > > + * > > + * The design of fwctl allows this sort of disassociation of the driver from the > > + * subsystem primarily by keeping memory allocations owned by the core subsytem. > > + * The fwctl_device and fwctl_uctx can both be freed without requiring a driver > > + * callback. This allows the module to remain unlocked while FDs are open. > > + */ And this explains the above a 2nd way > > +void fwctl_unregister(struct fwctl_device *fwctl) > > +{ > > + cdev_device_del(&fwctl->cdev, &fwctl->dev); > > + > > + /* > > + * The driver module may unload after this returns, the op pointer will > > + * not be valid. > > + */ > > + fwctl->ops = NULL; > I'd bring that in with the logic doing close_uctx() etc as then it will align > with the comments that I'd also suggest only adding there (patch 2 I think). Ok > > +/** > > + * fwctl_alloc_device - Allocate a fwctl > > + * @parent: Physical device that provides the FW interface > > + * @ops: Driver ops to register > > + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device > > + * @member: Name of the struct fwctl_device in @drv_struct > > + * > > + * This allocates and initializes the fwctl_device embedded in the drv_struct. > > + * Upon success the pointer must be freed via fwctl_put(). Returns NULL on > > + * failure. Returns a 'drv_struct *' on success, NULL on error. > > + */ > > +#define fwctl_alloc_device(parent, ops, drv_struct, member) \ > > + container_of(_fwctl_alloc_device( \ > > + parent, ops, \ > > + sizeof(drv_struct) + \ > > + BUILD_BUG_ON_ZERO( \ > > + offsetof(drv_struct, member))), \ > Doesn't that fire a build_bug when the member is at the start of drv_struct? > Or do I have that backwards? BUILD_BUG_ON(true) == failure, evaluates to void BUILD_BUG_ON_ZERO(true) == fails, evaluates to 0 BUILD_BUG_ON_ZERO(false) == false, evaluates to 0 It is a bit confusing name, it is not ON_ZERO it is BUG_ON return ZERO > Does container_of() safely handle a NULL? Generally no, nor does it handle ERR_PTR, but it does work for both if the offset is 0. The BUILD_BUG guarentees the 0 offset both so that the casting inside _fwctl_alloc_device() works and we can use safely use container_of() to enforce the type check. What do you think about writing it like this instead: #define fwctl_alloc_device(parent, ops, drv_struct, member) \ ({ \ static_assert(__same_type(struct fwctl_device, \ ((drv_struct *)NULL)->member)); \ static_assert(offsetof(drv_struct, member) == 0); \ (drv_struct *)_fwctl_alloc_device(parent, ops, \ sizeof(drv_struct)); \ }) ? In some ways I like it better.. Thanks, Jason
> > > > +/** > > > + * fwctl_alloc_device - Allocate a fwctl > > > + * @parent: Physical device that provides the FW interface > > > + * @ops: Driver ops to register > > > + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device > > > + * @member: Name of the struct fwctl_device in @drv_struct > > > + * > > > + * This allocates and initializes the fwctl_device embedded in the drv_struct. > > > + * Upon success the pointer must be freed via fwctl_put(). Returns NULL on > > > + * failure. Returns a 'drv_struct *' on success, NULL on error. > > > + */ > > > +#define fwctl_alloc_device(parent, ops, drv_struct, member) \ > > > + container_of(_fwctl_alloc_device( \ > > > + parent, ops, \ > > > + sizeof(drv_struct) + \ > > > + BUILD_BUG_ON_ZERO( \ > > > + offsetof(drv_struct, member))), \ > > Doesn't that fire a build_bug when the member is at the start of drv_struct? > > Or do I have that backwards? > > BUILD_BUG_ON(true) == failure, evaluates to void > BUILD_BUG_ON_ZERO(true) == fails, evaluates to 0 > BUILD_BUG_ON_ZERO(false) == false, evaluates to 0 > > It is a bit confusing name, it is not ON_ZERO it is BUG_ON return ZERO Ah. That indeed got me. ouch. > > > Does container_of() safely handle a NULL? > > Generally no, nor does it handle ERR_PTR, but it does work for both if > the offset is 0. Ah. Good point, I'd neglected the zero offset meaning it is really just a fancy pointer type cast. > > The BUILD_BUG guarentees the 0 offset both so that the casting inside > _fwctl_alloc_device() works and we can use safely use container_of() > to enforce the type check. > > What do you think about writing it like this instead: > > #define fwctl_alloc_device(parent, ops, drv_struct, member) \ > ({ \ > static_assert(__same_type(struct fwctl_device, \ > ((drv_struct *)NULL)->member)); \ > static_assert(offsetof(drv_struct, member) == 0); \ > (drv_struct *)_fwctl_alloc_device(parent, ops, \ > sizeof(drv_struct)); \ > }) > > ? > > In some ways I like it better.. Seems more readable to me and avoids entertaining corners of the previous approach. Jonathan > > Thanks, > Jason
diff --git a/MAINTAINERS b/MAINTAINERS index 2ca8f35dfe0399..aa7a760d12f8ef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9076,6 +9076,14 @@ F: kernel/futex/* F: tools/perf/bench/futex* F: tools/testing/selftests/futex/ +FWCTL SUBSYSTEM +M: Jason Gunthorpe <jgg@nvidia.com> +M: Saeed Mahameed <saeedm@nvidia.com> +S: Maintained +F: Documentation/userspace-api/fwctl.rst +F: drivers/fwctl/ +F: include/linux/fwctl.h + GALAXYCORE GC0308 CAMERA SENSOR DRIVER M: Sebastian Reichel <sre@kernel.org> L: linux-media@vger.kernel.org diff --git a/drivers/Kconfig b/drivers/Kconfig index 7bdad836fc6207..7c556c5ac4fddc 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -21,6 +21,8 @@ source "drivers/connector/Kconfig" source "drivers/firmware/Kconfig" +source "drivers/fwctl/Kconfig" + source "drivers/gnss/Kconfig" source "drivers/mtd/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index fe9ceb0d2288ad..f6a241b747b29c 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -133,6 +133,7 @@ obj-$(CONFIG_MEMSTICK) += memstick/ obj-y += leds/ obj-$(CONFIG_INFINIBAND) += infiniband/ obj-y += firmware/ +obj-$(CONFIG_FWCTL) += fwctl/ obj-$(CONFIG_CRYPTO) += crypto/ obj-$(CONFIG_SUPERH) += sh/ obj-y += clocksource/ diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig new file mode 100644 index 00000000000000..37147a695add9a --- /dev/null +++ b/drivers/fwctl/Kconfig @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0-only +menuconfig FWCTL + tristate "fwctl device firmware access framework" + help + fwctl provides a userspace API for restricted access to communicate + with on-device firmware. The communication channel is intended to + support a wide range of lockdown compatible device behaviors including + manipulating device FLASH, debugging, and other activities that don't + fit neatly into an existing subsystem. diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile new file mode 100644 index 00000000000000..1cad210f6ba580 --- /dev/null +++ b/drivers/fwctl/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_FWCTL) += fwctl.o + +fwctl-y += main.o diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c new file mode 100644 index 00000000000000..6e9bf15c743b5c --- /dev/null +++ b/drivers/fwctl/main.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES + */ +#define pr_fmt(fmt) "fwctl: " fmt +#include <linux/fwctl.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/container_of.h> +#include <linux/fs.h> + +enum { + FWCTL_MAX_DEVICES = 256, +}; +static dev_t fwctl_dev; +static DEFINE_IDA(fwctl_ida); + +static int fwctl_fops_open(struct inode *inode, struct file *filp) +{ + struct fwctl_device *fwctl = + container_of(inode->i_cdev, struct fwctl_device, cdev); + + get_device(&fwctl->dev); + filp->private_data = fwctl; + return 0; +} + +static int fwctl_fops_release(struct inode *inode, struct file *filp) +{ + struct fwctl_device *fwctl = filp->private_data; + + fwctl_put(fwctl); + return 0; +} + +static const struct file_operations fwctl_fops = { + .owner = THIS_MODULE, + .open = fwctl_fops_open, + .release = fwctl_fops_release, +}; + +static void fwctl_device_release(struct device *device) +{ + struct fwctl_device *fwctl = + container_of(device, struct fwctl_device, dev); + + ida_free(&fwctl_ida, fwctl->dev.devt - fwctl_dev); + kfree(fwctl); +} + +static char *fwctl_devnode(const struct device *dev, umode_t *mode) +{ + return kasprintf(GFP_KERNEL, "fwctl/%s", dev_name(dev)); +} + +static struct class fwctl_class = { + .name = "fwctl", + .dev_release = fwctl_device_release, + .devnode = fwctl_devnode, +}; + +static struct fwctl_device * +_alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size) +{ + struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL); + int devnum; + + if (!fwctl) + return NULL; + fwctl->dev.class = &fwctl_class; + fwctl->dev.parent = parent; + + devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL); + if (devnum < 0) + return NULL; + fwctl->dev.devt = fwctl_dev + devnum; + + device_initialize(&fwctl->dev); + return_ptr(fwctl); +} + +/* Drivers use the fwctl_alloc_device() wrapper */ +struct fwctl_device *_fwctl_alloc_device(struct device *parent, + const struct fwctl_ops *ops, + size_t size) +{ + struct fwctl_device *fwctl __free(fwctl) = + _alloc_device(parent, ops, size); + + if (!fwctl) + return NULL; + + cdev_init(&fwctl->cdev, &fwctl_fops); + fwctl->cdev.owner = THIS_MODULE; + + if (dev_set_name(&fwctl->dev, "fwctl%d", fwctl->dev.devt - fwctl_dev)) + return NULL; + + fwctl->ops = ops; + return_ptr(fwctl); +} +EXPORT_SYMBOL_NS_GPL(_fwctl_alloc_device, FWCTL); + +/** + * fwctl_register - Register a new device to the subsystem + * @fwctl: Previously allocated fwctl_device + * + * On return the device is visible through sysfs and /dev, driver ops may be + * called. + */ +int fwctl_register(struct fwctl_device *fwctl) +{ + int ret; + + ret = cdev_device_add(&fwctl->cdev, &fwctl->dev); + if (ret) + return ret; + return 0; +} +EXPORT_SYMBOL_NS_GPL(fwctl_register, FWCTL); + +/** + * fwctl_unregister - Unregister a device from the subsystem + * @fwctl: Previously allocated and registered fwctl_device + * + * Undoes fwctl_register(). On return no driver ops will be called. The + * caller must still call fwctl_put() to free the fwctl. + * + * Unregister will return even if userspace still has file descriptors open. + * This will call ops->close_uctx() on any open FDs and after return no driver + * op will be called. The FDs remain open but all fops will return -ENODEV. + * + * The design of fwctl allows this sort of disassociation of the driver from the + * subsystem primarily by keeping memory allocations owned by the core subsytem. + * The fwctl_device and fwctl_uctx can both be freed without requiring a driver + * callback. This allows the module to remain unlocked while FDs are open. + */ +void fwctl_unregister(struct fwctl_device *fwctl) +{ + cdev_device_del(&fwctl->cdev, &fwctl->dev); + + /* + * The driver module may unload after this returns, the op pointer will + * not be valid. + */ + fwctl->ops = NULL; +} +EXPORT_SYMBOL_NS_GPL(fwctl_unregister, FWCTL); + +static int __init fwctl_init(void) +{ + int ret; + + ret = alloc_chrdev_region(&fwctl_dev, 0, FWCTL_MAX_DEVICES, "fwctl"); + if (ret) + return ret; + + ret = class_register(&fwctl_class); + if (ret) + goto err_chrdev; + return 0; + +err_chrdev: + unregister_chrdev_region(fwctl_dev, FWCTL_MAX_DEVICES); + return ret; +} + +static void __exit fwctl_exit(void) +{ + class_unregister(&fwctl_class); + unregister_chrdev_region(fwctl_dev, FWCTL_MAX_DEVICES); +} + +module_init(fwctl_init); +module_exit(fwctl_exit); +MODULE_DESCRIPTION("fwctl device firmware access framework"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h new file mode 100644 index 00000000000000..ef4eaa87c945e4 --- /dev/null +++ b/include/linux/fwctl.h @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES + */ +#ifndef __LINUX_FWCTL_H +#define __LINUX_FWCTL_H +#include <linux/device.h> +#include <linux/cdev.h> +#include <linux/cleanup.h> + +struct fwctl_device; +struct fwctl_uctx; + +struct fwctl_ops { +}; + +/** + * struct fwctl_device - Per-driver registration struct + * @dev: The sysfs (class/fwctl/fwctlXX) device + * + * Each driver instance will have one of these structs with the driver + * private data following immeidately after. This struct is refcounted, + * it is freed by calling fwctl_put(). + */ +struct fwctl_device { + struct device dev; + /* private: */ + struct cdev cdev; + const struct fwctl_ops *ops; +}; + +struct fwctl_device *_fwctl_alloc_device(struct device *parent, + const struct fwctl_ops *ops, + size_t size); +/** + * fwctl_alloc_device - Allocate a fwctl + * @parent: Physical device that provides the FW interface + * @ops: Driver ops to register + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device + * @member: Name of the struct fwctl_device in @drv_struct + * + * This allocates and initializes the fwctl_device embedded in the drv_struct. + * Upon success the pointer must be freed via fwctl_put(). Returns NULL on + * failure. Returns a 'drv_struct *' on success, NULL on error. + */ +#define fwctl_alloc_device(parent, ops, drv_struct, member) \ + container_of(_fwctl_alloc_device( \ + parent, ops, \ + sizeof(drv_struct) + \ + BUILD_BUG_ON_ZERO( \ + offsetof(drv_struct, member))), \ + drv_struct, member) + +static inline struct fwctl_device *fwctl_get(struct fwctl_device *fwctl) +{ + get_device(&fwctl->dev); + return fwctl; +} +static inline void fwctl_put(struct fwctl_device *fwctl) +{ + put_device(&fwctl->dev); +} +DEFINE_FREE(fwctl, struct fwctl_device *, if (_T) fwctl_put(_T)); + +int fwctl_register(struct fwctl_device *fwctl); +void fwctl_unregister(struct fwctl_device *fwctl); + +#endif
Create the class, character device and functions for a fwctl driver to un/register to the subsystem. A typical fwctl driver has a sysfs presence like: $ ls -l /dev/fwctl/fwctl0 crw------- 1 root root 250, 0 Apr 25 19:16 /dev/fwctl/fwctl0 $ ls /sys/class/fwctl/fwctl0 dev device power subsystem uevent $ ls /sys/class/fwctl/fwctl0/device/infiniband/ ibp0s10f0 $ ls /sys/class/infiniband/ibp0s10f0/device/fwctl/ fwctl0/ $ ls /sys/devices/pci0000:00/0000:00:0a.0/fwctl/fwctl0 dev device power subsystem uevent Which allows userspace to link all the multi-subsystem driver components together and learn the subsystem specific names for the device's components. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- MAINTAINERS | 8 ++ drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/fwctl/Kconfig | 9 +++ drivers/fwctl/Makefile | 4 + drivers/fwctl/main.c | 177 +++++++++++++++++++++++++++++++++++++++++ include/linux/fwctl.h | 68 ++++++++++++++++ 7 files changed, 269 insertions(+) create mode 100644 drivers/fwctl/Kconfig create mode 100644 drivers/fwctl/Makefile create mode 100644 drivers/fwctl/main.c create mode 100644 include/linux/fwctl.h