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

[Dave Jiang <dave.jiang@intel.com>]

On 11/21/24 11:20 AM, Jonathan Cameron wrote:
> 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).

If 32bit alignment is fine then I'll drop it.

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