| Message ID | 3-v4-0cf4ec3b8143+4995-fwctl_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce fwctl subystem | expand |
On Thu, 6 Feb 2025 20:13:25 -0400 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. > > Allow the device to provide a device specific info struct that contains > any additional information that the driver may need to provide to > userspace. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Trivial comment inline. > --- > drivers/fwctl/main.c | 51 ++++++++++++++++++++++++++++++++++++++ > include/linux/fwctl.h | 12 +++++++++ > include/uapi/fwctl/fwctl.h | 32 ++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) > > diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c > index d561deaf2b86d8..4b6792f2031e86 100644 > --- a/drivers/fwctl/main.c > +++ b/drivers/fwctl/main.c > @@ -27,8 +27,58 @@ struct fwctl_ucmd { > diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h > index f4718a6240f281..ac66853200a5a8 100644 > --- a/include/uapi/fwctl/fwctl.h > +++ b/include/uapi/fwctl/fwctl.h > @@ -4,6 +4,9 @@ > #ifndef _UAPI_FWCTL_H > #define _UAPI_FWCTL_H > > +#include <linux/types.h> > +#include <linux/ioctl.h> > + > #define FWCTL_TYPE 0x9A > > /** > @@ -33,6 +36,35 @@ > */ > enum { > FWCTL_CMD_BASE = 0, > + FWCTL_CMD_INFO = 0, > + FWCTL_CMD_RPC = 1, Trivial but in theory should perhaps leave RPC for patch 5? > };
On Fri, Feb 07, 2025 at 01:06:41PM +0000, Jonathan Cameron wrote: > > enum { > > FWCTL_CMD_BASE = 0, > > + FWCTL_CMD_INFO = 0, > > + FWCTL_CMD_RPC = 1, > > Trivial but in theory should perhaps leave RPC for patch 5? Yeah, I think it is a rebasing error Thanks, Jason
On 2/6/25 5:13 PM, Jason Gunthorpe 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. > > Allow the device to provide a device specific info struct that contains > any additional information that the driver may need to provide to > userspace. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/fwctl/main.c | 51 ++++++++++++++++++++++++++++++++++++++ > include/linux/fwctl.h | 12 +++++++++ > include/uapi/fwctl/fwctl.h | 32 ++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) > > diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c > index d561deaf2b86d8..4b6792f2031e86 100644 > --- a/drivers/fwctl/main.c > +++ b/drivers/fwctl/main.c > @@ -27,8 +27,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) = > + 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 { > @@ -48,6 +98,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 93b470efb9dbc3..9b6cc8ae1aa0ca 100644 > --- a/include/linux/fwctl.h > +++ b/include/linux/fwctl.h > @@ -7,6 +7,7 @@ > #include <linux/device.h> > #include <linux/cdev.h> > #include <linux/cleanup.h> > +#include <uapi/fwctl/fwctl.h> > > struct fwctl_device; > struct fwctl_uctx; > @@ -19,6 +20,10 @@ struct fwctl_uctx; > * it will block device hot unplug and module unloading. > */ > struct fwctl_ops { > + /** > + * @device_type: The drivers assigned device_type number. This is uABI. > + */ > + enum fwctl_device_type device_type; > /** > * @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 > @@ -35,6 +40,13 @@ struct fwctl_ops { > * is closed. > */ > void (*close_uctx)(struct fwctl_uctx *uctx); > + /** > + * @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. > + */ > + void *(*info)(struct fwctl_uctx *uctx, size_t *length); > }; > > /** > diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h > index f4718a6240f281..ac66853200a5a8 100644 > --- a/include/uapi/fwctl/fwctl.h > +++ b/include/uapi/fwctl/fwctl.h > @@ -4,6 +4,9 @@ > #ifndef _UAPI_FWCTL_H > #define _UAPI_FWCTL_H > > +#include <linux/types.h> > +#include <linux/ioctl.h> > + > #define FWCTL_TYPE 0x9A > > /** > @@ -33,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
diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c index d561deaf2b86d8..4b6792f2031e86 100644 --- a/drivers/fwctl/main.c +++ b/drivers/fwctl/main.c @@ -27,8 +27,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) = + 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 { @@ -48,6 +98,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 93b470efb9dbc3..9b6cc8ae1aa0ca 100644 --- a/include/linux/fwctl.h +++ b/include/linux/fwctl.h @@ -7,6 +7,7 @@ #include <linux/device.h> #include <linux/cdev.h> #include <linux/cleanup.h> +#include <uapi/fwctl/fwctl.h> struct fwctl_device; struct fwctl_uctx; @@ -19,6 +20,10 @@ struct fwctl_uctx; * it will block device hot unplug and module unloading. */ struct fwctl_ops { + /** + * @device_type: The drivers assigned device_type number. This is uABI. + */ + enum fwctl_device_type device_type; /** * @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 @@ -35,6 +40,13 @@ struct fwctl_ops { * is closed. */ void (*close_uctx)(struct fwctl_uctx *uctx); + /** + * @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. + */ + void *(*info)(struct fwctl_uctx *uctx, size_t *length); }; /** diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h index f4718a6240f281..ac66853200a5a8 100644 --- a/include/uapi/fwctl/fwctl.h +++ b/include/uapi/fwctl/fwctl.h @@ -4,6 +4,9 @@ #ifndef _UAPI_FWCTL_H #define _UAPI_FWCTL_H +#include <linux/types.h> +#include <linux/ioctl.h> + #define FWCTL_TYPE 0x9A /** @@ -33,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