Re: [PATCH v3 03/10] fwctl: FWCTL_INFO to return basic information about the device

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

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);
>  };
>  
>  /**