On Wed, Oct 18, 2017 at 8:58 AM, Jiri Pirko <[email protected]> wrote:
> Wed, Oct 18, 2017 at 02:39:09PM CEST, [email protected] wrote:
>>On Wed, Oct 18, 2017 at 3:11 AM, Jiri Pirko <[email protected]> wrote:
>>> Tue, Oct 17, 2017 at 10:44:23PM CEST, [email protected] wrote:
>>> Steve. As I originally requested, could you please split this to:
>>> 1) single patch adding config get/set commands, without any config
>>> attributes
>>> 2) single patch per config attribute - please don't add them in bulk.
>>> We also need very strict description for every single attribute so
>>> other vendors know what it is and can re-use it. There is need to
>>> avoid duplication here. Also, please send just few attribites in the
>>> first run, not like 40 you are sending now. Impossible to review.
>>
>>I broke the patch set up into functional blocks of attributes, in
>>order to avoid having ~40 patches of just a couple lines each. But, I
>>will split further for each individual attribute, and just submit a
>>few initially, per your request.
>>
>>>
>>> Also, why didn't you put it into nested attribute we were discussing?
>>>
>>
>>I thought I did :) , using the DPIPE_HEADERS nested attribute as an
>>example. I'll reach out to you off-list to understand what I'm
>>missing.
>
> I missed that. But you need a separate attr enum as well.
>
I did have this as the nested attr enum in the original patch:
/* Permanent Configuration Parameters */
DEVLINK_ATTR_PERM_CFG, /* nested */
However, I only used the nested construct in the response from kernel
to userspace, not in the request from userspace to kernel. (This was
based on looking at the various DPIPE_* nested attributes as
examples.)
Thinking about it after seeing your comment, I'm thinking I should
also use the nested attribute construct in the original request from
userspace to kernel as well, although I didn't see any previous
examples of this in devlink.
So I'll plan to use nesting in that direction as well.
Thanks,
Steve
