| Message ID | 3-v2-940e479ceba9+3821-fwctl_jgg@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce fwctl subystem | expand |
On Mon, 24 Jun 2024 19:47:27 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > Userspace will need to know some details about the fwctl interface being > used to locate the correct userspace code to communicate with the > kernel. Provide a simple device_type enum indicating what the kernel > driver is. As below - maybe consider a UUID? Would let you decouple allocating those with upstreaming drivers. We'll just get annoying races on the enum otherwise as multiple drivers get upstreamed that use this. > > Allow the device to provide a device specific info struct that contains > any additional information that the driver may need to provide to > userspace. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/fwctl/main.c | 53 ++++++++++++++++++++++++++++++++++++++ > include/linux/fwctl.h | 8 ++++++ > include/uapi/fwctl/fwctl.h | 29 +++++++++++++++++++++ > 3 files changed, 90 insertions(+) > > diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c > index 6872c01d5c62e8..f1dec0b590aee4 100644 > --- a/drivers/fwctl/main.c > +++ b/drivers/fwctl/main.c > @@ -17,6 +17,8 @@ enum { > static dev_t fwctl_dev; > static DEFINE_IDA(fwctl_ida); > > +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T)); Why need for a new one? That's the same as the one in slab.h from 6.9 onwards. Before that it was if (_T) I was going to suggest promoting this to slab.h and then found the normal implementation had been improved since I last checked. > > /** > diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h > index 0bdce95b6d69d9..39db9f09f8068e 100644 > --- a/include/uapi/fwctl/fwctl.h > +++ b/include/uapi/fwctl/fwctl.h > @@ -36,6 +36,35 @@ > */ > enum { > FWCTL_CMD_BASE = 0, > + FWCTL_CMD_INFO = 0, > + FWCTL_CMD_RPC = 1, > }; > > +enum fwctl_device_type { > + FWCTL_DEVICE_TYPE_ERROR = 0, > +}; > + > +/** > + * struct fwctl_info - ioctl(FWCTL_INFO) > + * @size: sizeof(struct fwctl_info) > + * @flags: Must be 0 > + * @out_device_type: Returns the type of the device from enum fwctl_device_type Maybe a UUID? Avoid need to synchronize that list for ever. > + * @device_data_len: On input the length of the out_device_data memory. On > + * output the size of the kernel's device_data which may be larger or > + * smaller than the input. Maybe 0 on input. > + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will > + * fill the entire memory, zeroing as required. Why do we need device in names of these two? > + * > + * Returns basic information about this fwctl instance, particularly what driver > + * is being used to define the device_data format. > + */ > +struct fwctl_info { > + __u32 size; > + __u32 flags; > + __u32 out_device_type; > + __u32 device_data_len; > + __aligned_u64 out_device_data; > +}; > +#define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO) > + > #endif
On Fri, Jul 26, 2024 at 04:15:03PM +0100, Jonathan Cameron wrote: > On Mon, 24 Jun 2024 19:47:27 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > Userspace will need to know some details about the fwctl interface being > > used to locate the correct userspace code to communicate with the > > kernel. Provide a simple device_type enum indicating what the kernel > > driver is. > > As below - maybe consider a UUID? > Would let you decouple allocating those with upstreaming drivers. > We'll just get annoying races on the enum otherwise as multiple > drivers get upstreamed that use this. I view the coupling as a feature - controlling uABI number assignment is one of the subtle motivations the kernel community has typically used to encourage upstream participation. > > static dev_t fwctl_dev; > > static DEFINE_IDA(fwctl_ida); > > > > +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T)); > > Why need for a new one? That's the same as the one in slab.h from > 6.9 onwards. Before that it was > if (_T) > > I was going to suggest promoting this to slab.h and then found > the normal implementation had been improved since I last checked. Same, now it is improved it can go away. > > +/** > > + * struct fwctl_info - ioctl(FWCTL_INFO) > > + * @size: sizeof(struct fwctl_info) > > + * @flags: Must be 0 > > + * @out_device_type: Returns the type of the device from enum fwctl_device_type > > Maybe a UUID? Avoid need to synchronize that list for ever. > > > + * @device_data_len: On input the length of the out_device_data memory. On > > + * output the size of the kernel's device_data which may be larger or > > + * smaller than the input. Maybe 0 on input. > > + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will > > + * fill the entire memory, zeroing as required. > > Why do we need device in names of these two? I'm not sure I understand this question? out_device_type returns the "name" out_device_data returns a struct of data, the layout of the struct is defined by out_device_type Jason
On Mon, 29 Jul 2024 13:35:13 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Fri, Jul 26, 2024 at 04:15:03PM +0100, Jonathan Cameron wrote: > > On Mon, 24 Jun 2024 19:47:27 -0300 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > Userspace will need to know some details about the fwctl interface being > > > used to locate the correct userspace code to communicate with the > > > kernel. Provide a simple device_type enum indicating what the kernel > > > driver is. > > > > As below - maybe consider a UUID? > > Would let you decouple allocating those with upstreaming drivers. > > We'll just get annoying races on the enum otherwise as multiple > > drivers get upstreamed that use this. > > I view the coupling as a feature - controlling uABI number assignment > is one of the subtle motivations the kernel community has typically > used to encourage upstream participation. Hmm. I'm not sure it's worth the possible pain if this becomes popular. Maybe you'll have to run a reservation hotline. > > > > +/** > > > + * struct fwctl_info - ioctl(FWCTL_INFO) > > > + * @size: sizeof(struct fwctl_info) > > > + * @flags: Must be 0 > > > + * @out_device_type: Returns the type of the device from enum fwctl_device_type > > > > Maybe a UUID? Avoid need to synchronize that list for ever. > > > > > + * @device_data_len: On input the length of the out_device_data memory. On > > > + * output the size of the kernel's device_data which may be larger or > > > + * smaller than the input. Maybe 0 on input. > > > + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will > > > + * fill the entire memory, zeroing as required. > > > > Why do we need device in names of these two? > > I'm not sure I understand this question? > > out_device_type returns the "name" > > out_device_data returns a struct of data, the layout of the struct is > defined by out_device_type What is device in this case? fwctl struct device, hardware device, something else? I'm not sure what the names give over fwctl_type, out_data_len, out_data The first one can't just be type as likely as not out_data contains a type field specific to the fwctl_device_type. I don't feel that strongly about this though, so stick to device if you like. I'll get used to it. Jonathan > > Jason
On Tue, Jul 30, 2024 at 06:34:41PM +0100, Jonathan Cameron wrote: > On Mon, 29 Jul 2024 13:35:13 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Fri, Jul 26, 2024 at 04:15:03PM +0100, Jonathan Cameron wrote: > > > On Mon, 24 Jun 2024 19:47:27 -0300 > > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > > Userspace will need to know some details about the fwctl interface being > > > > used to locate the correct userspace code to communicate with the > > > > kernel. Provide a simple device_type enum indicating what the kernel > > > > driver is. > > > > > > As below - maybe consider a UUID? > > > Would let you decouple allocating those with upstreaming drivers. > > > We'll just get annoying races on the enum otherwise as multiple > > > drivers get upstreamed that use this. > > > > I view the coupling as a feature - controlling uABI number assignment > > is one of the subtle motivations the kernel community has typically > > used to encourage upstream participation. > > Hmm. I'm not sure it's worth the possible pain if this becomes > popular. Maybe you'll have to run a reservation hotline. As long as there is one tree things get sorted. We'd need a big scale of new drivers for this to be a practical problem. Big incentive for people to get their stuff merged before shipping it. :) > > > > + * @device_data_len: On input the length of the out_device_data memory. On > > > > + * output the size of the kernel's device_data which may be larger or > > > > + * smaller than the input. Maybe 0 on input. > > > > + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will > > > > + * fill the entire memory, zeroing as required. > > > > > > Why do we need device in names of these two? > > > > I'm not sure I understand this question? > > > > out_device_type returns the "name" > > > > out_device_data returns a struct of data, the layout of the struct is > > defined by out_device_type > > What is device in this case? fwctl struct device, hardware device, something else? Oh, I see what you are asking.. fwctl is split into a common ABI and a "device" ABI which varies depending on the fwctl driver. So The labeling was to put "device" in front of those things that vary. Basically if you touch a "device" field you need a userspace driver that understands that device. Not sure it is worth to have an explicit naming, it is sort of a RDMAism creeping in where we called this concept "driver_data" Jason
diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c index 6872c01d5c62e8..f1dec0b590aee4 100644 --- a/drivers/fwctl/main.c +++ b/drivers/fwctl/main.c @@ -17,6 +17,8 @@ enum { static dev_t fwctl_dev; static DEFINE_IDA(fwctl_ida); +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T)); + struct fwctl_ucmd { struct fwctl_uctx *uctx; void __user *ubuffer; @@ -24,8 +26,58 @@ struct fwctl_ucmd { u32 user_size; }; +static int ucmd_respond(struct fwctl_ucmd *ucmd, size_t cmd_len) +{ + if (copy_to_user(ucmd->ubuffer, ucmd->cmd, + min_t(size_t, ucmd->user_size, cmd_len))) + return -EFAULT; + return 0; +} + +static int copy_to_user_zero_pad(void __user *to, const void *from, + size_t from_len, size_t user_len) +{ + size_t copy_len; + + copy_len = min(from_len, user_len); + if (copy_to_user(to, from, copy_len)) + return -EFAULT; + if (copy_len < user_len) { + if (clear_user(to + copy_len, user_len - copy_len)) + return -EFAULT; + } + return 0; +} + +static int fwctl_cmd_info(struct fwctl_ucmd *ucmd) +{ + struct fwctl_device *fwctl = ucmd->uctx->fwctl; + struct fwctl_info *cmd = ucmd->cmd; + size_t driver_info_len = 0; + + if (cmd->flags) + return -EOPNOTSUPP; + + if (cmd->device_data_len) { + void *driver_info __free(kfree_errptr) = + fwctl->ops->info(ucmd->uctx, &driver_info_len); + if (IS_ERR(driver_info)) + return PTR_ERR(driver_info); + + if (copy_to_user_zero_pad(u64_to_user_ptr(cmd->out_device_data), + driver_info, driver_info_len, + cmd->device_data_len)) + return -EFAULT; + } + + cmd->out_device_type = fwctl->ops->device_type; + cmd->device_data_len = driver_info_len; + return ucmd_respond(ucmd, sizeof(*cmd)); +} + /* On stack memory for the ioctl structs */ union ucmd_buffer { + struct fwctl_info info; }; struct fwctl_ioctl_op { @@ -45,6 +97,7 @@ struct fwctl_ioctl_op { .execute = _fn, \ } static const struct fwctl_ioctl_op fwctl_ioctl_ops[] = { + IOCTL_OP(FWCTL_INFO, fwctl_cmd_info, struct fwctl_info, out_device_data), }; static long fwctl_fops_ioctl(struct file *filp, unsigned int cmd, diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h index 1d9651de92fc19..9a906b861acf3a 100644 --- a/include/linux/fwctl.h +++ b/include/linux/fwctl.h @@ -7,12 +7,14 @@ #include <linux/device.h> #include <linux/cdev.h> #include <linux/cleanup.h> +#include <uapi/fwctl/fwctl.h> struct fwctl_device; struct fwctl_uctx; /** * struct fwctl_ops - Driver provided operations + * @device_type: The drivers assigned device_type number. This is uABI * @uctx_size: The size of the fwctl_uctx struct to allocate. The first * bytes of this memory will be a fwctl_uctx. The driver can use the * remaining bytes as its private memory. @@ -20,11 +22,17 @@ struct fwctl_uctx; * used. * @close_uctx: Called when the uctx is destroyed, usually when the FD is * closed. + * @info: Implement FWCTL_INFO. Return a kmalloc() memory that is copied to + * out_device_data. On input length indicates the size of the user buffer + * on output it indicates the size of the memory. The driver can ignore + * length on input, the core code will handle everything. */ struct fwctl_ops { + enum fwctl_device_type device_type; size_t uctx_size; int (*open_uctx)(struct fwctl_uctx *uctx); void (*close_uctx)(struct fwctl_uctx *uctx); + void *(*info)(struct fwctl_uctx *uctx, size_t *length); }; /** diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h index 0bdce95b6d69d9..39db9f09f8068e 100644 --- a/include/uapi/fwctl/fwctl.h +++ b/include/uapi/fwctl/fwctl.h @@ -36,6 +36,35 @@ */ enum { FWCTL_CMD_BASE = 0, + FWCTL_CMD_INFO = 0, + FWCTL_CMD_RPC = 1, }; +enum fwctl_device_type { + FWCTL_DEVICE_TYPE_ERROR = 0, +}; + +/** + * struct fwctl_info - ioctl(FWCTL_INFO) + * @size: sizeof(struct fwctl_info) + * @flags: Must be 0 + * @out_device_type: Returns the type of the device from enum fwctl_device_type + * @device_data_len: On input the length of the out_device_data memory. On + * output the size of the kernel's device_data which may be larger or + * smaller than the input. Maybe 0 on input. + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will + * fill the entire memory, zeroing as required. + * + * Returns basic information about this fwctl instance, particularly what driver + * is being used to define the device_data format. + */ +struct fwctl_info { + __u32 size; + __u32 flags; + __u32 out_device_type; + __u32 device_data_len; + __aligned_u64 out_device_data; +}; +#define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO) + #endif
Userspace will need to know some details about the fwctl interface being used to locate the correct userspace code to communicate with the kernel. Provide a simple device_type enum indicating what the kernel driver is. Allow the device to provide a device specific info struct that contains any additional information that the driver may need to provide to userspace. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/fwctl/main.c | 53 ++++++++++++++++++++++++++++++++++++++ include/linux/fwctl.h | 8 ++++++ include/uapi/fwctl/fwctl.h | 29 +++++++++++++++++++++ 3 files changed, 90 insertions(+)