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

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

On 4/12/2023 2:36 PM, Shannon Nelson wrote:
> On 4/12/23 9:58 AM, Tantilov, Emil S wrote:
>>
>> On 4/10/2023 3:12 PM, Shannon Nelson wrote:
>>> 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.
>>>
>> We can certainly look into it, but for the purpose of this series it
>> doesn't seem like a meaningful change if it only helps with the grouping
>> since the define names already follow a certain pattern to indicate that
>> they are related.
> 
> I was trying not to be overly pedantic, but the last words of that 
> paragraph are copied directly from section 12 of the coding-style.rst. 
> We should follow the wisdom therein.
> 
> Look, whether we like this patchset or not, it is going to get used as 
> an example and a starting point for related work, so we need to be sure 
> it serves as a good example.  Let's start from the beginning with clean 
> code.

Got it. Will convert to enums in v3.

> 
>>
>>>
>>> [...]
>>>
>>>>
>>>>>> +
>>>>>> +/* 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.
>>
>> OK, we'll remove it.
> 
> I'm not sure 'it' means your synonym or the actual check.  The check is 
> a useful thing to help make sure no one screws up the API message 
> layout, so don't remove the check itself.  If you can't get away with 
> ignoring the checkpatch.pl CHECK complaint about the line spacing, I'm 
> fine with leaving it alone.  Some other day we can look at teaching 
> checkpatch.pl to allow static_assert() after a struct.
>

I should have been more specific. I was referring to removing the blank 
line as I think we can live with the CHECK. Your call I guess.

>>
>>>
>>>
>>> [...]
>>>
>>>>>> +/* 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.
>>
>> We can change the name to reserved to make it clearer, but the size
>> cannot be changed because it's an ABI already.
> 
> That's fine - just make sure it is clear this was intended.

Right.

Thanks for the review,
Emil

> 
> sln