| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZXB0oyBiE0C/5laE@nanopsycho>
Date: Wed, 6 Dec 2023 14:18:27 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, netdev@...r.kernel.org,
linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH net-next v5] netlink: specs: devlink: add some(not all)
missing attributes in devlink.yaml
Sat, Dec 02, 2023 at 01:30:48PM CET, swarupkotikalapudi@...il.com wrote:
>Add some missing(not all) attributes in devlink.yaml.
>
>Re-generate the related devlink-user.[ch] code.
>
>Signed-off-by: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@...il.com>
>Suggested-by: Jiri Pirko <jiri@...nulli.us>
>Suggested-by: Jakub Kicinski <kuba@...nel.org>
>Fixes: f2f9dd164db0 ("netlink: specs: devlink: add the remaining command to generate complete split_ops")
Remove the fixes tag. This patch is not a fix.
Overall, looks okay. I found couple of issues below, also pointed out
couple of nits to change:
>---
>V5:
> - Keep stats enum as unnamed in /uapi/linux/devlink.h
> to avoid kernel build failure
>V4: https://lore.kernel.org/all/20231126105246.195288-1-swarupkotikalapudi@gmail.com/
> - Change the commit message
>V3: https://lore.kernel.org/all/20231123100119.148324-1-swarupkotikalapudi@gmail.com/
> - enum name added for stats and trap-metadata enum used by trap command
> in include/uapi/linux/devlink.h
> - Fix generated userspace file's compilation issue
> due to V1 and V2 patchset
> - Move some attributes e.g. nested-devlink and param again as a TODO,
> which needs some discussion and will be fixed in a new patchset
>V2: https://lore.kernel.org/all/20231122143033.89856-1-swarupkotikalapudi@gmail.com/
> - Rebase to net-next tree
> - param-value-data data type is dynamic, hence to accomndate
> all data type make it as string type
> - Change nested attribute to use correct fields
> based on driver code e.g. region-snapshots,
> region-snapshot, region-chunks, region-chunk,
> linecard-supported-types, health-reporter,
> linecard-supported-types, nested-devlink
> and param's attributes
>V1: https://lore.kernel.org/all/ZVNPi7pmJIDJ6Ms7@swarup-virtual-machine/
>
> Documentation/netlink/specs/devlink.yaml | 330 ++++++++++++++++++-----
> tools/net/ynl/generated/devlink-user.c | 223 +++++++++++++++
> tools/net/ynl/generated/devlink-user.h | 105 +++++++-
> 3 files changed, 595 insertions(+), 63 deletions(-)
>
>diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
>index 43067e1f63aa..2fee16509e82 100644
>--- a/Documentation/netlink/specs/devlink.yaml
>+++ b/Documentation/netlink/specs/devlink.yaml
>@@ -75,6 +75,14 @@ definitions:
> name: ipsec-crypto-bit
> -
> name: ipsec-packet-bit
>+ -
>+ type: enum
>+ name: rate-type
>+ entries:
>+ -
>+ name: leaf
>+ -
>+ name: node
> -
> type: enum
> name: sb-threshold-type
>@@ -111,6 +119,16 @@ definitions:
> name: none
> -
> name: basic
>+ -
>+ type: enum
>+ name: dpipe-header-id
>+ entries:
>+ -
>+ name: ethernet
>+ -
>+ name: ipv4
>+ -
>+ name: ipv6
> -
> type: enum
> name: dpipe-match-type
>@@ -174,7 +192,16 @@ definitions:
> name: trap
> -
> name: mirror
>-
>+ -
>+ type: enum
>+ name: trap-type
>+ entries:
>+ -
>+ name: drop
>+ -
>+ name: exception
>+ -
>+ name: control
> attribute-sets:
> -
> name: devlink
>@@ -194,23 +221,44 @@ attribute-sets:
> name: port-type
> type: u16
> enum: port-type
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: port-desired-type
>+ type: u16
>+ -
>+ name: port-netdev-ifindex
>+ type: u32
>+ -
>+ name: port-netdev-name
>+ type: string
>+ -
>+ name: port-ibdev-name
>+ type: string
> -
> name: port-split-count
> type: u32
> value: 9
In general, when you remove fill-up the gaps, please remove the "value"
from the next attr after the gap. It is no longer needed. This applies
to all the gaps you fill in this file.
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: port-split-group
>+ type: u32
> -
> name: sb-index
> type: u32
> value: 11
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: sb-size
>+ type: u32
>+ -
>+ name: sb-ingress-pool-count
>+ type: u16
>+ -
>+ name: sb-egress-pool-count
>+ type: u16
>+ -
>+ name: sb-ingress-tc-count
>+ type: u16
>+ -
>+ name: sb-egress-tc-count
>+ type: u16
> -
> name: sb-pool-index
> type: u16
>@@ -233,15 +281,17 @@ attribute-sets:
> name: sb-tc-index
> type: u16
> value: 22
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: sb-occ-cur
>+ type: u32
>+ -
>+ name: sb-occ-max
>+ type: u32
> -
> name: eswitch-mode
> type: u16
> value: 25
> enum: eswitch-mode
>-
> -
> name: eswitch-inline-mode
> type: u16
>@@ -347,6 +397,7 @@ attribute-sets:
> -
> name: dpipe-header-id
> type: u32
>+ enum: dpipe-header-id
> -
> name: dpipe-header-fields
> type: nest
>@@ -433,23 +484,27 @@ attribute-sets:
> name: port-flavour
> type: u16
> enum: port-flavour
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: port-number
>+ type: u32
>+ -
>+ name: port-split-subport-number
>+ type: u32
>+ -
>+ name: param
>+ type: nest
>+ nested-attributes: dl-param
> -
> name: param-name
> type: string
> value: 81
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: param-generic
>+ type: flag
> -
> name: param-type
> type: u8
> value: 83
>-
>- # TODO: fill in the attributes in between
>-
Don't remove this TODO comment. You still are missing couple of
attributes here.
> -
> name: param-value-cmode
> type: u8
>@@ -458,16 +513,32 @@ attribute-sets:
> -
> name: region-name
> type: string
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: region-size
>+ type: u64
>+ -
>+ name: region-snapshots
>+ type: nest
>+ nested-attributes: dl-region-snapshots
>+ -
>+ name: region-snapshot
>+ type: nest
>+ nested-attributes: dl-region-snapshot
> -
> name: region-snapshot-id
> type: u32
> value: 92
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: region-chunks
>+ type: nest
>+ nested-attributes: dl-region-chunks
>+ -
>+ name: region-chunk
>+ type: nest
>+ nested-attributes: dl-region-chunk
>+ -
>+ name: region-chunk-data
>+ type: binary
> -
> name: region-chunk-addr
> type: u64
>@@ -502,9 +573,9 @@ attribute-sets:
> -
> name: info-version-value
> type: string
>-
>- # TODO: fill in the attributes in between
>-
>+ -
>+ name: sb-pool-cell-size
>+ type: u32
> -
> name: fmsg
> type: nest
>@@ -525,15 +596,31 @@ attribute-sets:
> -
> name: fmsg-obj-name
> type: string
>-
>+ -
>+ name: fmsg-obj-value-type
>+ type: u8
Empty line there please.
> # TODO: fill in the attributes in between
Empty line there please.
>+ -
>+ name: health-reporter
>+ type: nest
>+ nested-attributes: dl-health-reporter
You need to specify "value: 114" for this attr, there is still a gap
in front of it.
>
> -
> name: health-reporter-name
> type: string
> value: 115
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: health-reporter-state
>+ type: u8
>+ -
>+ name: health-reporter-err-count
>+ type: u64
>+ -
>+ name: health-reporter-recover-count
>+ type: u64
>+ -
>+ name: health-reporter-dump-ts
>+ type: u64
>
> -
> name: health-reporter-graceful-period
>@@ -548,15 +635,27 @@ attribute-sets:
> -
> name: flash-update-component
> type: string
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: flash-update-status-msg
>+ type: string
>+ -
>+ name: flash-update-status-done
>+ type: u64
>+ -
>+ name: flash-update-status-total
>+ type: u64
>
Could you please make sure that the consistency in empty lines between
attributes is maintained after this patch is applied. For example, the
empty line above should be removed. Please check the rest of the file.
> -
> name: port-pci-pf-number
> type: u16
> value: 127
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: port-pci-vf-number
>+ type: u16
>+ -
>+ name: stats
>+ type: nest
>+ nested-attributes: dl-attr-stats
>
> -
> name: trap-name
>@@ -566,8 +665,17 @@ attribute-sets:
> name: trap-action
> type: u8
> enum: trap-action
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: trap-type
>+ type: u8
>+ enum: trap-type
>+ -
>+ name: trap-generic
>+ type: flag
>+ -
>+ name: trap-metadata
>+ type: nest
>+ nested-attributes: dl-trap-metadata
>
> -
> name: trap-group-name
>@@ -577,8 +685,9 @@ attribute-sets:
> -
> name: reload-failed
> type: u8
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: health-reporter-dump-ts-ns
>+ type: u64
>
> -
> name: netns-fd
>@@ -591,8 +700,6 @@ attribute-sets:
> name: netns-id
> type: u32
>
>- # TODO: fill in the attributes in between
>-
> -
> name: health-reporter-auto-dump
> type: u8
>@@ -610,15 +717,26 @@ attribute-sets:
> name: port-function
> type: nest
> nested-attributes: dl-port-function
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: info-board-serial-number
>+ type: string
>+ -
>+ name: port-lanes
>+ type: u32
>+ -
>+ name: port-splittable
>+ type: u8
>+ -
>+ name: port-external
>+ type: u8
>
> -
> name: port-controller-number
> type: u32
> value: 150
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: flash-update-status-timeout
>+ type: u64
>
> -
> name: flash-update-overwrite-mask
>@@ -674,14 +792,14 @@ attribute-sets:
> multi-attr: true
> nested-attributes: dl-reload-act-stats
>
>- # TODO: fill in the attributes in between
>-
> -
> name: port-pci-sf-number
> type: u32
> value: 164
>-
>- # TODO: fill in the attributes in between
>+ -
>+ name: rate-type
>+ type: u16
>+ enum: rate-type
>
> -
> name: rate-tx-share
>@@ -697,22 +815,28 @@ attribute-sets:
> name: rate-parent-node-name
> type: string
>
>- # TODO: fill in the attributes in between
>+ -
>+ name: region-max-snapshots
>+ type: u32
>
> -
> name: linecard-index
> type: u32
> value: 171
>
>- # TODO: fill in the attributes in between
>+ -
>+ name: linecard-state
>+ type: u8
>
> -
> name: linecard-type
> type: string
> value: 173
>-
>+ -
>+ name: linecard-supported-types
>+ type: nest
>+ nested-attributes: dl-linecard-supported-types
Empty line here please.
> # TODO: fill in the attributes in between
>-
Don't remove the empty line.
> -
> name: selftests
> type: nest
>@@ -727,7 +851,6 @@ attribute-sets:
> -
> name: region-direct
> type: flag
>-
Don't remove the empty line.
> -
> name: dl-dev-stats
> subset-of: devlink
>@@ -1004,7 +1127,43 @@ attribute-sets:
> attributes:
> -
> name: resource
>-
Again, could you please maintain consistency with empty lines between
the subsets?
>+ -
>+ name: dl-param
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: param-name
>+ -
>+ name: param-generic
>+ -
>+ name: param-type
>+ # TODO: fill in the attribute param-value-list
>+ -
>+ name: dl-region-snapshots
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: region-snapshot
>+ -
>+ name: dl-region-snapshot
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: region-snapshot-id
>+ -
>+ name: dl-region-chunks
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: region-chunk
>+ -
>+ name: dl-region-chunk
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: region-chunk-data
>+ -
>+ name: region-chunk-addr
> -
> name: dl-fmsg
> subset-of: devlink
>@@ -1019,7 +1178,58 @@ attribute-sets:
> name: fmsg-nest-end
> -
> name: fmsg-obj-name
>-
>+ -
>+ name: dl-health-reporter
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: health-reporter-name
>+ -
>+ name: health-reporter-state
>+ -
>+ name: health-reporter-err-count
>+ -
>+ name: health-reporter-recover-count
>+ -
>+ name: health-reporter-graceful-period
>+ -
>+ name: health-reporter-auto-recover
>+ -
>+ name: health-reporter-dump-ts
>+ -
>+ name: health-reporter-dump-ts-ns
>+ -
>+ name: health-reporter-auto-dump
>+ -
>+ name: dl-attr-stats
>+ name-prefix: devlink-attr-
>+ attributes:
>+ - name: stats-rx-packets
>+ type: u64
>+ value: 0
Why is 0 needed here?
>+ -
>+ name: stats-rx-bytes
>+ type: u64
>+ -
>+ name: stats-rx-dropped
>+ type: u64
>+ -
>+ name: dl-trap-metadata
>+ name-prefix: devlink-attr-
>+ attributes:
>+ -
>+ name: trap-metadata-type-in-port
>+ type: flag
>+ value: 0
Why is 0 needed here?
>+ -
>+ name: trap-metadata-type-fa-cookie
>+ type: flag
>+ -
>+ name: dl-linecard-supported-types
>+ subset-of: devlink
>+ attributes:
>+ -
>+ name: linecard-type
> -
> name: dl-selftest-id
> name-prefix: devlink-attr-selftest-id-
[...]
Powered by blists - more mailing lists