Re: [RFC PATCH v2 11/20] fwctl: FWCTL_HW_INFO to return hardware information

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Nov 15, 2024 at 02:25:44PM -0700, Dave Jiang wrote:
> --- a/include/linux/fwctl.h
> +++ b/include/linux/fwctl.h
> @@ -47,6 +47,12 @@ struct fwctl_ops {
>  	 * ignore length on input, the core code will handle everything.
>  	 */
>  	void *(*info)(struct fwctl_uctx *uctx, size_t *length);
> +	/**
> +	 * @hw_info: Implement FWCTL_HW_INFO. Return a kmalloc() memory that is
> +	 * copied to out_device_dataa. On input commands indicates the number

Spelling 'data'

> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 4e4d30104667..7334907e27c1 100644
> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -37,7 +37,8 @@
>  enum {
>  	FWCTL_CMD_BASE = 0,
>  	FWCTL_CMD_INFO = 0,
> -	FWCTL_CMD_RPC = 1,
> +	FWCTL_CMD_HW_INFO = 1,
> +	FWCTL_CMD_RPC = 2,
>  };

Let's follow uapi practicies from the start, give HW_INFO 2 instead of
renumbering rpc (or we need to order this patch before RPC)

> +/**
> + * struct fwctl_command_info - Hardware command information returned from a query.
> + * @id: Driver ID number for the command
> + * @opcode: Hardware command opcode
> + * @set_effects: Effects to the OS and hardware when command is executed.
> + *               Provided by the device.

This seems like it needs a common set of bit definitions if it is
going to be in a common struct

> + * @size_in: Expected input size, or ~0U if variable length.
> + * @size_out: Expected output size, or ~0U if variable length.
> + *
> + * Represents a single command that is supported by both the driver and the
> + * hardware. This is returned as part of an array from the FWCTL_HW_INFO ioctl.
> + */
> +struct fwctl_command_info {
> +	__u32 id;
> +	__u16 opcode;
> +	__u16 effects;
> +	__u32 size_in;
> +	__u32 size_out;
> +};

What is the difference btween id and opcode? That should be explained
in the comment

I'd suggest to make the raw HW id to be u64 'just in case'

> +/**
> + * struct fwctl_hw_info - ioctl(FWCTL_HW_INFO)
> + * @size: sizeof(struct fwctl_hw_info)
> + * @flags: Must be 0
> + * @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.
> + * @nr_cmds: Number of commands requested. 0 returns only nr_cmds in output.
> + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will
> + *	fill the entire memory, zeroing as required.
> + *
> + * Returns hardware commands information about this fwctl instance.
> + */
> +struct fwctl_hw_info {
> +	__u32 size;
> +	__u32 flags;
> +	__u32 device_data_len;
> +	__u32 nr_cmds;
> +	__aligned_u64 out_device_data;
> +};

It isn't really device data, so this should be called out_hw_info

Jason