On Mon, 29 Jul 2024 14:05:53 -0300
Jason Gunthorpe [off-list ref] wrote:

On Fri, Jul 26, 2024 at 04:01:57PM +0100, Jonathan Cameron wrote:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

 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.

quoted

quoted

 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.

quoted

quoted

@@ -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;

quoted

quoted

+/**
+ * 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