Re: [Intel-wired-lan] [PATCH net-next 01/15] virtchnl: add virtchnl version 2 ops

[Shannon Nelson <shannon.nelson@amd.com>]

On 4/10/23 1:27 PM, Linga, Pavan Kumar wrote:
> 
> On 4/4/2023 3:31 AM, Shannon Nelson wrote:
>> On 3/29/23 7:03 AM, Pavan Kumar Linga wrote:
>>>
>>> Virtchnl version 1 is an interface used by the current generation of
>>> foundational NICs to negotiate the capabilities and configure the
>>> HW resources such as queues, vectors, RSS LUT, etc between the PF
>>> and VF drivers. It is not extensible to enable new features supported
>>> in the next generation of NICs/IPUs and to negotiate descriptor types,
>>> packet types and register offsets.
>>>

[...]

>>> +
>>> +#include "virtchnl2_lan_desc.h"
>>> +
>>> +/* VIRTCHNL2_ERROR_CODES */
>>> +/* Success */
>>> +#define VIRTCHNL2_STATUS_SUCCESS       0
>>
>> Shouldn't these be enum and not #define?
>>
> 
> This header file is describing communication protocol with opcodes,
> error codes, capabilities etc. that are exchanged between IDPF and
> device Control Plane. Compiler chooses the size of the enum based on the
> enumeration constants that are present which is not a constant size. But
> for virtchnl protocol, we want to have fixed size no matter what. To
> avoid such cases, we are using defines whereever necessary.

The field size limitations in an API are one thing, and that can be 
managed by using a u8/u16/u32 or whatever as necessary.  But that 
doesn't mean that you can't define values to be assigned in those fields 
as enums, which are preferred when defining several related constants.


[...]

> 
>>> +
>>> +/* VIRTCHNL2_OP_GET_EDT_CAPS
>>> + * Get EDT granularity and time horizon
>>> + */
>>> +struct virtchnl2_edt_caps {
>>> +       /* Timestamp granularity in nanoseconds */
>>> +       __le64 tstamp_granularity_ns;
>>> +       /* Total time window in nanoseconds */
>>> +       __le64 time_horizon_ns;
>>> +};
>>> +
>>> +VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_edt_caps);
>>
>> Don't put a space between the struct and the check.
>>
> 
> Checkpatch reports a warning (actually a 'Check') when the newline is
> removed. Following is the checkpatch output when the newline is removed:
> 
> "CHECK: Please use a blank line after function/struct/union/enum
> declarations"

Since it has to do directly with the finished definition, one would 
think it could follow the same rule as EXPORT... does.  It might not be 
a bad idea at some point for static_assert() to be added to that allowed 
list.  For now, though, since it is only a CHECK and not WARN or ERROR, 
you might be able to ignore it.  It might be easier to ignore if you 
just used the existing static_assert() rather than definigin your own 
synonym.


[...]

>>> +/* Queue to vector mapping */
>>> +struct virtchnl2_queue_vector {
>>> +       __le32 queue_id;
>>> +       __le16 vector_id;
>>> +       u8 pad[2];
>>> +
>>> +       /* See VIRTCHNL2_ITR_IDX definitions */
>>> +       __le32 itr_idx;
>>> +
>>> +       /* See VIRTCHNL2_QUEUE_TYPE definitions */
>>> +       __le32 queue_type;
>>> +       u8 pad1[8];
>>> +};
>>
>> Why the end padding?  What's wrong with the 16-byte size?
>>
> 
> The end padding is added for any possible future additions of the fields
> to this structure. Didn't get the ask for 16-byte size, can you please
> elaborate?

Without the pad1[8], this struct is an even 16 bytes, seems like a 
logical place to stop.  24 bytes seems odd, if you're going to pad for 
the future it makes some sense to do it to an even 32 bytes 
(power-of-2).  And please add a comment for this future thinking.

sln