On Mon, 24 Jun 2024 19:47:25 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx>
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)
> +
