| Message ID | 3-v3-960f17f90f17+516-fwctl_jgg@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce fwctl subystem | expand |
On Wed, 21 Aug 2024 15:10:55 -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. > > 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> Just one minor question: How likely is the data being passed back from the driver to be const? Feels like it might be and should be easy enough to support either const or not. Either way, LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h > index ca4245825e91bf..6b596931a55169 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. > + */ Maybe it's worth supporting const data as well? > + void *(*info)(struct fwctl_uctx *uctx, size_t *length); > }; > > /**
On Fri, Aug 23, 2024 at 03:14:11PM +0100, Jonathan Cameron wrote: > On Wed, 21 Aug 2024 15:10:55 -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. > > > > 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> > > Just one minor question: How likely is the data being passed back > from the driver to be const? I'm guessing not very? I expect alot of drivers will want to include dynamic information about their FW > Feels like it might be and should > be easy enough to support either const or not. It would by easy, lets wait and see, adding another op is trivial. Allocating memory is not the end of the world on this path anyhow. Thanks, Jason
On Tue, Aug 27, 2024 at 11:47:23AM -0300, Jason Gunthorpe wrote: > On Fri, Aug 23, 2024 at 03:14:11PM +0100, Jonathan Cameron wrote: > > On Wed, 21 Aug 2024 15:10:55 -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. > > > > > > 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> > > > > Just one minor question: How likely is the data being passed back > > from the driver to be const? > > I'm guessing not very? I expect alot of drivers will want to include > dynamic information about their FW > Agreed. The presumption is that this will be used to query information from FW that has no existing API to discover the values. > > Feels like it might be and should > > be easy enough to support either const or not. > > It would by easy, lets wait and see, adding another op is trivial. > Allocating memory is not the end of the world on this path anyhow. +1 > > Thanks, > Jason
diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c index f2e30ffc1e0cb5..b281ccc52b4e57 100644 --- a/drivers/fwctl/main.c +++ b/drivers/fwctl/main.c @@ -25,8 +25,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 { @@ -46,6 +96,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 ca4245825e91bf..6b596931a55169 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 22fa750d7e8184..39db9f09f8068e 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
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 | 51 ++++++++++++++++++++++++++++++++++++++ include/linux/fwctl.h | 12 +++++++++ include/uapi/fwctl/fwctl.h | 32 ++++++++++++++++++++++++ 3 files changed, 95 insertions(+)