On Mon, 24 Jun 2024 19:47:27 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
