Message-ID: <15ca06c3-8344-0396-1bb4-38f219a31369@amd.com>
Date:   Thu, 13 Apr 2023 14:03:01 -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/13/23 11:54 AM, Tantilov, Emil S wrote:
> 
> 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.

Thanks

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

I'd prefer to live with the CHECK as long as Jakub and friends are good 
with it.

sln


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

Powered by blists - more mailing lists