Date:   Wed, 12 Apr 2023 14:36:50 -0700
From:   Shannon Nelson <shannon.nelson@....com>
To:     "Tantilov, Emil S" <emil.s.tantilov@...el.com>,
        "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>,
        intel-wired-lan@...ts.osuosl.org
Cc:     netdev@...r.kernel.org, shiraz.saleem@...el.com,
        willemb@...gle.com, decot@...gle.com, joshua.a.hay@...el.com,
        sridhar.samudrala@...el.com, Alan Brady <alan.brady@...el.com>,
        Madhu Chittim <madhu.chittim@...el.com>,
        Phani Burra <phani.r.burra@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next 01/15] virtchnl: add virtchnl
 version 2 ops

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.


> 
>>
>> [...]
>>
>>>
>>>>> +
>>>>> +/* 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.

> 
>>
>>
>> [...]
>>
>>>>> +/* 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.

sln

Powered by blists - more mailing lists