From: Shannon Nelson <shannon.nelson@amd•com>
To: "Tantilov, Emil S" <emil.s.tantilov@intel•com>,
"Linga, Pavan Kumar" <pavan.kumar.linga@intel•com>,
intel-wired-lan@lists•osuosl.org
Cc: netdev@vger•kernel.org, shiraz.saleem@intel•com,
willemb@google•com, decot@google•com, joshua.a.hay@intel•com,
sridhar.samudrala@intel•com, Alan Brady <alan.brady@intel•com>,
Madhu Chittim <madhu.chittim@intel•com>,
Phani Burra <phani.r.burra@intel•com>
Subject: Re: [Intel-wired-lan] [PATCH net-next 01/15] virtchnl: add virtchnl version 2 ops
Date: Wed, 12 Apr 2023 14:36:50 -0700 [thread overview]
Message-ID: <ffe43a28-641c-c263-2ea2-67882b476cde@amd.com> (raw)
In-Reply-To: <ffd66203-4349-0986-2130-f8b156f1923a@intel.com>
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
next prev parent reply other threads:[~2023-04-12 21:37 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-29 14:03 [Intel-wired-lan] [PATCH net-next 00/15] Introduce IDPF driver Pavan Kumar Linga
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 01/15] virtchnl: add virtchnl version 2 ops Pavan Kumar Linga
2023-03-31 15:25 ` Simon Horman
2023-04-03 22:01 ` Shannon Nelson
2023-04-03 22:20 ` Jakub Kicinski
2023-04-03 22:54 ` Shannon Nelson
2023-04-03 23:30 ` Jakub Kicinski
2023-04-04 7:59 ` Orr, Michael
2023-04-04 16:25 ` Shannon Nelson
2023-04-04 19:41 ` Orr, Michael
2023-04-10 20:27 ` Linga, Pavan Kumar
2023-04-10 22:12 ` Shannon Nelson
2023-04-12 16:58 ` Tantilov, Emil S
2023-04-12 21:36 ` Shannon Nelson [this message]
2023-04-13 18:54 ` Tantilov, Emil S
2023-04-13 21:03 ` Shannon Nelson
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 02/15] idpf: add module register and probe functionality Pavan Kumar Linga
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 03/15] idpf: add controlq init and reset checks Pavan Kumar Linga
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 04/15] idpf: add core init and interrupt request Pavan Kumar Linga
2023-03-31 15:39 ` Simon Horman
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 05/15] idpf: add create vport and netdev configuration Pavan Kumar Linga
2023-03-31 15:46 ` Simon Horman
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 06/15] idpf: continue expanding init task Pavan Kumar Linga
2023-03-31 15:47 ` Simon Horman
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 07/15] idpf: configure resources for TX queues Pavan Kumar Linga
2023-03-31 15:49 ` Simon Horman
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 08/15] idpf: configure resources for RX queues Pavan Kumar Linga
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 09/15] idpf: initialize interrupts and enable vport Pavan Kumar Linga
2023-03-31 15:59 ` Simon Horman
2023-04-04 19:36 ` Linga, Pavan Kumar
2023-04-05 10:07 ` Simon Horman
2023-03-29 14:03 ` [Intel-wired-lan] [PATCH net-next 10/15] idpf: add splitq start_xmit Pavan Kumar Linga
2023-03-29 14:04 ` [Intel-wired-lan] [PATCH net-next 11/15] idpf: add TX splitq napi poll support Pavan Kumar Linga
2023-03-29 14:04 ` [Intel-wired-lan] [PATCH net-next 12/15] idpf: add RX " Pavan Kumar Linga
2023-03-30 16:23 ` Maciej Fijalkowski
2023-04-05 0:51 ` Tantilov, Emil S
2023-03-29 14:04 ` [Intel-wired-lan] [PATCH net-next 13/15] idpf: add singleq start_xmit and napi poll Pavan Kumar Linga
2023-03-29 14:04 ` [Intel-wired-lan] [PATCH net-next 14/15] idpf: add ethtool callbacks Pavan Kumar Linga
2023-03-29 15:33 ` Andrew Lunn
2023-03-30 22:05 ` Linga, Pavan Kumar
2023-03-29 14:04 ` [Intel-wired-lan] [PATCH net-next 15/15] idpf: configure SRIOV and add other ndo_ops Pavan Kumar Linga
2023-03-29 15:41 ` [Intel-wired-lan] [PATCH net-next 00/15] Introduce IDPF driver Paul Menzel
2023-03-30 21:31 ` Linga, Pavan Kumar
2023-03-29 17:31 ` Willem de Bruijn
2023-03-30 12:03 ` Jason Gunthorpe
2023-03-30 17:25 ` Jakub Kicinski
2023-03-30 18:29 ` Jason Gunthorpe
2023-04-03 21:36 ` Samudrala, Sridhar
2023-04-04 16:42 ` Jason Gunthorpe
2023-04-04 19:19 ` Orr, Michael
2023-04-04 23:35 ` Jason Gunthorpe
2023-04-07 4:39 ` Christoph Hellwig
2023-04-07 18:01 ` Shannon Nelson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ffe43a28-641c-c263-2ea2-67882b476cde@amd.com \
--to=shannon.nelson@amd$(echo .)com \
--cc=alan.brady@intel$(echo .)com \
--cc=decot@google$(echo .)com \
--cc=emil.s.tantilov@intel$(echo .)com \
--cc=intel-wired-lan@lists$(echo .)osuosl.org \
--cc=joshua.a.hay@intel$(echo .)com \
--cc=madhu.chittim@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pavan.kumar.linga@intel$(echo .)com \
--cc=phani.r.burra@intel$(echo .)com \
--cc=shiraz.saleem@intel$(echo .)com \
--cc=sridhar.samudrala@intel$(echo .)com \
--cc=willemb@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox