On Mon, 29 Jul 2024 14:05:53 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Fri, Jul 26, 2024 at 04:01:57PM +0100, Jonathan Cameron wrote:
>
> > > +struct fwctl_ioctl_op {
> > > + unsigned int size;
> > > + unsigned int min_size;
> > > + unsigned int ioctl_num;
> > > + int (*execute)(struct fwctl_ucmd *ucmd);
> > > +};
> > > +
> > > +#define IOCTL_OP(_ioctl, _fn, _struct, _last) \
> > > + [_IOC_NR(_ioctl) - FWCTL_CMD_BASE] = { \
> >
> > If this is always zero indexed, maybe just drop the - FWCTL_CMD_BASE here
> > and elsewhere? Maybe through in a BUILD_BUG to confirm it is always 0.
>
> I left it like this in case someone had different ideas for the number
> space (iommufd uses a non 0 base also). I think either is fine, and I
> slightly prefer keeping it rather than a static_assert.
Ok. Feels a little messy to me. But fair enough I guess.
> > > + if (!uctx)
> > > + return -ENOMEM;
> > > +
> > > + uctx->fwctl = fwctl;
> > > + ret = fwctl->ops->open_uctx(uctx);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + scoped_guard(mutex, &fwctl->uctx_list_lock) {
> > > + list_add_tail(&uctx->uctx_list_entry, &fwctl->uctx_list);
> > > + }
> >
> > I guess more may come later but do we need {}?
>
> I guessed the extra {} would be style guide for this construct?
Maybe. Not seen any statements on that yet, and it's becoming
quite common.
>
> > > static int fwctl_fops_release(struct inode *inode, struct file *filp)
> > > {
> > > - struct fwctl_device *fwctl = filp->private_data;
> > > + struct fwctl_uctx *uctx = filp->private_data;
> > > + struct fwctl_device *fwctl = uctx->fwctl;
> > >
> > > + scoped_guard(rwsem_read, &fwctl->registration_lock) {
> > > + if (fwctl->ops) {
> >
> > Maybe a comment on when this path happens to help the reader
> > along. (when the file is closed and device is still alive).
> > Otherwise was cleaned up already in fwctl_unregister()
>
> scoped_guard(rwsem_read, &fwctl->registration_lock) {
> /*
> * fwctl_unregister() has already removed the driver and
> * destroyed the uctx.
> */
> if (fwctl->ops) {
>
Good.
> > > 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);
> > > +
> >
> > Obviously it's a little more heavy weight but I'd just use
> > list_for_each_entry_safe()
> >
> > Less effort for reviewers than consider the custom iteration
> > you are doing instead.
>
> For these constructs the goal is the make the list empty, it is a
> tinsy bit safer/clearer to drive the list to empty purposefully rather
> than iterate over it and hope it is empty once done.
>
> However there is no possible way that list_for_each_entry_safe() would
> be an unsafe construct here. I can change it if you feel strongly
Meh. You get to maintain this if it flies, so your choice.
>
> > > @@ -26,6 +39,10 @@ struct fwctl_device {
> > > struct device dev;
> > > /* private: */
> > > struct cdev cdev;
> > > +
> > > + struct rw_semaphore registration_lock;
> > > + struct mutex uctx_list_lock;
> >
> > Even for private locks, a scope statement would
> > be good to have.
>
> Like so?
>
> /*
> * Protect ops, held for write when ops becomes NULL during unregister,
> * held for read whenver ops is loaded or an ops function is running.
That does the job nicely.
> */
> struct rw_semaphore registration_lock;
> /* Protect uctx_list */
> struct mutex uctx_list_lock;
>
> > > +/**
> > > + * 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
> > > + * the structure in the first u32. The kernel checks that any structure space
> > > + * beyond what it understands is 0. This allows userspace to use the backward
> > > + * compatible portion while consistently using the newer, larger, structures.
> >
> > Is that particularly helpful? Userspace needs to know not to put anything in
> > those fields, not hard for it to also know what the size it should send is?
> > The two will change together.
>
> It is very helpful for a practical userspace.
>
> Lets say we have an ioctl struct:
>
> struct fwctl_info {
> __u32 size;
> __u32 flags;
> __u32 out_device_type;
> __u32 device_data_len;
> __aligned_u64 out_device_data;
> };
>
> And the basic userspace pattern is:
>
> struct fwctl_info info = {.size = sizeof(info), ...);
> ioctl(fd, FWCTL_INFO, &info);
>
> This works today and generates the 24 byte command.
>
> Tomorrow the kernel adds a new member:
>
> struct fwctl_info {
> __u32 size;
> __u32 flags;
> __u32 out_device_type;
> __u32 device_data_len;
> __aligned_u64 out_device_data;
> __aligned_u64 new_thing;
> };
>
> Current builds of the userpace use a 24 byte command. A new kernel
> will see the 24 bytes and behave as before.
>
> When I recompile the userspace with the updated header it will issue a
> 32 byte command with no source change.
>
> Old kernel will see a 32 byte command with the trailing bytes it
> doesn't understand as 0 and keep working.
>
> The new kernel will see the new_thing bytes are zero and behave the
> same as before.
>
> If then the userspace decides to set new_thing the old kernel will
> stop working. Userspace can use some 'try and fail' approach to try
> again with new_thing = 0.
I'm not keen on try and fail interfaces because they become messy
if this has potentially be extended multiple times. Rest
of argument is fair enough. Thanks for the explanation.
>
> It gives a whole bunch of easy paths for userspace, otherwise
> userspace has to be very careful to match the size of the struct to
> the ABI it is targetting. Realistically nobody will do that right.
>
> Thanks,
> Jason
