Re: [PATCH v2 1/8] fwctl: Add basic structure for a class subsystem with a cdev

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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)
> +