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
