Re: [PATCH v2 3/8] fwctl: FWCTL_INFO to return basic information about the device

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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