[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <761f0f12-417e-400d-b8f1-ccb9c8766c09@intel.com>
Date: Fri, 7 Feb 2025 17:21:08 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Andy Gospodarek <andrew.gospodarek@...adcom.com>,
Aron Silverton <aron.silverton@...cle.com>,
Dan Williams <dan.j.williams@...el.com>,
Daniel Vetter <daniel.vetter@...ll.ch>, David Ahern <dsahern@...nel.org>,
Andy Gospodarek <gospo@...adcom.com>, Christoph Hellwig <hch@...radead.org>,
Itay Avraham <itayavr@...dia.com>, Jiri Pirko <jiri@...dia.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Jakub Kicinski <kuba@...nel.org>, Leonid Bloch <lbloch@...dia.com>,
Leon Romanovsky <leonro@...dia.com>, linux-cxl@...r.kernel.org,
linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
Saeed Mahameed <saeedm@...dia.com>, "Nelson, Shannon"
<shannon.nelson@....com>
Subject: Re: [PATCH v4 03/10] fwctl: FWCTL_INFO to return basic information
about the device
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@...wei.com>
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
Reviewed-by: Dave Jiang <dave.jiang@...el.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
Powered by blists - more mailing lists