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

[Jason Gunthorpe <jgg@nvidia.com>]

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