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

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

On Fri, 15 Nov 2024 14:25:44 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add an optional ioctl FWCTL_HW_INFO to pass command specific information
> to user space. An array of 'struct fwctl_command_info' will be returned
> from the ioctl. These commands are send to the driver via FWCTL_RPC call.
> The command info struct contains the command id, the related hardware
> opcode, input and output size for the command, and the effects the command
> has if it's a write command.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Minor stuff inline.

Jonathan


> 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

>  enum fwctl_device_type {
> @@ -69,6 +70,61 @@ struct fwctl_info {
>  };
>  #define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO)
>  
> +/**
> + * 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.

Run kernel doc over the files.  (wrong name)

> + * @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;
> +};
> +
> +/**
> + * struct fwctl_hw_info_out - output struct for FWCTL_HW_INFO
> + * @nr_cmds: Number of commands for output
> + * @reserved: Reserved u32 for alignment.

As currently defined, why do we need to pad? I'm not against
padding just not understanding the comment.
It's more fun if there is a u64 in there (we all love 32 bit
x86 data alignment).

> + * @commands: Array of 'struct fwctl_command_info'
> + */
> +struct fwctl_hw_info_out {
> +	__u32 nr_cmds;
> +	__u32 reserved;
> +
> +	struct fwctl_command_info commands[] __counted_by(nr_cmds);
> +};