On Wed, 21 Aug 2024 15:10:54 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> Each file descriptor gets a chunk of per-FD driver specific context that
> allows the driver to attach a device specific struct to. The core code
> takes care of the memory lifetime for this structure.
>
> The ioctl dispatch and design is based on what was built for iommufd. The
> ioctls have a struct which has a combined in/out behavior with a typical
> 'zero pad' scheme for future extension and backwards compatibility.
>
> Like iommufd some shared logic does most of the ioctl marshalling and
> compatibility work and tables diatches to some function pointers for
> each unique iotcl.
>
> This approach has proven to work quite well in the iommufd and rdma
> subsystems.
>
> Allocate an ioctl number space for the subsystem.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Hi Jason,
A few minor things inline, but all trivial so
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c
> index 7f3e7713d0e6e9..f2e30ffc1e0cb5 100644
> --- a/drivers/fwctl/main.c
> +++ b/drivers/fwctl/main.c
>
> @@ -71,6 +183,9 @@ _alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
>
> fwctl->dev.class = &fwctl_class;
> fwctl->dev.parent = parent;
> + init_rwsem(&fwctl->registration_lock);
> + mutex_init(&fwctl->uctx_list_lock);
If the ida_alloc_max() fails,I don't think you destroy the mutex as the
device isn't yet initialized / put in the error path.
Whilst i find it hard to care, it's nice to always destroy mutex, or not do it at all.
Feels odd to only do it if things go well.
> + INIT_LIST_HEAD(&fwctl->uctx_list);
>
> devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL);
> if (devnum < 0)
> @@ -127,6 +242,10 @@ EXPORT_SYMBOL_NS_GPL(fwctl_register, FWCTL);
> * 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
> @@ -134,7 +253,23 @@ EXPORT_SYMBOL_NS_GPL(fwctl_register, FWCTL);
> */
> void fwctl_unregister(struct fwctl_device *fwctl)
> {
> + struct fwctl_uctx *uctx;
> +
> cdev_device_del(&fwctl->cdev, &fwctl->dev);
> +
> + /* Disable and free the driver's resources for any still open FDs. */
> + guard(rwsem_write)(&fwctl->registration_lock);
> + guard(mutex)(&fwctl->uctx_list_lock);
> + while ((uctx = list_first_entry_or_null(&fwctl->uctx_list,
> + struct fwctl_uctx,
> + uctx_list_entry)))
> + fwctl_destroy_uctx(uctx);
> +
> + /*
> + * 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);
>
> diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h
> index 68ac2d5ab87481..ca4245825e91bf 100644
> --- a/include/linux/fwctl.h
> +++ b/include/linux/fwctl.h
>
> /**
> @@ -26,6 +49,15 @@ struct fwctl_device {
> struct device dev;
> /* private: */
> struct cdev cdev;
> +
> + /*
> + * Protect ops, held for write when ops becomes NULL during unregister,
> + * held for read whenver ops is loaded or an ops function is running.
> + */
> + struct rw_semaphore registration_lock;
Maybe move down to just above ops?
> + /* Protect uctx_list */
> + struct mutex uctx_list_lock;
> + struct list_head uctx_list;
> const struct fwctl_ops *ops;
> };
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> new file mode 100644
> index 00000000000000..22fa750d7e8184
> --- /dev/null
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES.
> + */
> +#ifndef _UAPI_FWCTL_H
> +#define _UAPI_FWCTL_H
> +
> +#define FWCTL_TYPE 0x9A
> +
> +/**
> + * DOC: General ioctl format
> + *
> + * The ioctl interface follows a general format to allow for extensibility. Each
> + * ioctl is passed in a structure pointer as the argument providing the size of
Pedantic Englishman time:
passed a structure pointer
(otherwise I read that as passing an ioctl in a pointer which is weird).
> + * the structure in the first u32. The kernel checks that any structure space
...
