netdev - Re: [patch net-next RFC 00/10] introduce line card support for modular switch

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210118130331.GV3565223@nanopsycho.orion>
Date:   Mon, 18 Jan 2021 14:03:31 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        jacob.e.keller@...el.com, roopa@...dia.com, mlxsw@...dia.com
Subject: Re: [patch net-next RFC 00/10] introduce line card support for
 modular switch

Fri, Jan 15, 2021 at 07:01:45PM CET, idosch@...sch.org wrote:
>On Fri, Jan 15, 2021 at 05:55:59PM +0100, Jiri Pirko wrote:
>> Fri, Jan 15, 2021 at 04:43:57PM CET, idosch@...sch.org wrote:
>> >On Wed, Jan 13, 2021 at 01:12:12PM +0100, Jiri Pirko wrote:
>> >> # Create a new netdevsim device, with no ports and 2 line cards:
>> >> $ echo "10 0 2" >/sys/bus/netdevsim/new_device
>> >> $ devlink port # No ports are listed
>> >> $ devlink lc
>> >> netdevsim/netdevsim10:
>> >>   lc 0 state unprovisioned
>> >>     supported_types:
>> >>        card1port card2ports card4ports
>> >>   lc 1 state unprovisioned
>> >>     supported_types:
>> >>        card1port card2ports card4ports
>> >> 
>> >> # Note that driver advertizes supported line card types. In case of
>> >> # netdevsim, these are 3.
>> >> 
>> >> $ devlink lc provision netdevsim/netdevsim10 lc 0 type card4ports
>> >
>> >Why do we need a separate command for that? You actually introduced
>> >'DEVLINK_CMD_LINECARD_SET' in patch #1, but it's never used.
>> >
>> >I prefer:
>> >
>> >devlink lc set netdevsim/netdevsim10 index 0 state provision type card4ports
>> 
>> This is misleading. This is actually not setting state. The state gets
>> changed upon successful provisioning process. Also, one may think that
>> he can set other states, but he can't. I don't like this at all :/
>
>So make state a read-only attribute. You really only care about setting
>the type.
>
>To provision:
>
># devlink lc set netdevsim/netdevsim10 index 0 type card4ports
>
>To unprovsion:
>
># devlink lc set netdevsim/netdevsim10 index 0 type none
>
>Or:
>
># devlink lc set netdevsim/netdevsim10 index 0 notype

Hmm, okay, that might work. And I can add state "FAILED_PROVISION" what
would indicate that after the type was set by the user, driver was not
able to successfully provision. The the user has to set "notype" & "type
x" again. Sounds good?


>
>> 
>> 
>> >devlink lc set netdevsim/netdevsim10 index 0 state unprovision
>> >
>> >It is consistent with the GET/SET/NEW/DEL pattern used by other
>> >commands.
>> 
>> Not really, see split port for example. This is similar to that.
>
>It's not. The split command creates new objects whereas this command
>modifies an existing object.

You are right.


>
>> 
>> >
>> >> $ devlink lc
>> >> netdevsim/netdevsim10:
>> >>   lc 0 state provisioned type card4ports
>> >>     supported_types:
>> >>        card1port card2ports card4ports
>> >>   lc 1 state unprovisioned
>> >>     supported_types:
>> >>        card1port card2ports card4ports
>> >> $ devlink port
>> >> netdevsim/netdevsim10/1000: type eth netdev eni10nl0p1 flavour physical lc 0 port 1 splittable false
>> >> netdevsim/netdevsim10/1001: type eth netdev eni10nl0p2 flavour physical lc 0 port 2 splittable false
>> >> netdevsim/netdevsim10/1002: type eth netdev eni10nl0p3 flavour physical lc 0 port 3 splittable false
>> >> netdevsim/netdevsim10/1003: type eth netdev eni10nl0p4 flavour physical lc 0 port 4 splittable false
>> >> #                                                 ^^                    ^^^^
>> >> #                                     netdev name adjusted          index of a line card this port belongs to
>> >> 
>> >> $ ip link set eni10nl0p1 up 
>> >> $ ip link show eni10nl0p1   
>> >> 165: eni10nl0p1: <NO-CARRIER,BROADCAST,NOARP,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>> >>     link/ether 7e:2d:05:93:d3:d1 brd ff:ff:ff:ff:ff:ff
>> >> 
>> >> # Now activate the line card using debugfs. That emulates plug-in event
>> >> # on real hardware:
>> >> $ echo "Y"> /sys/kernel/debug/netdevsim/netdevsim10/linecards/0/active
>> >> $ ip link show eni10nl0p1
>> >> 165: eni10nl0p1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>> >>     link/ether 7e:2d:05:93:d3:d1 brd ff:ff:ff:ff:ff:ff
>> >> # The carrier is UP now.
>> >> 
>> >> Jiri Pirko (10):
>> >>   devlink: add support to create line card and expose to user
>> >>   devlink: implement line card provisioning
>> >>   devlink: implement line card active state
>> >>   devlink: append split port number to the port name
>> >>   devlink: add port to line card relationship set
>> >>   netdevsim: introduce line card support
>> >>   netdevsim: allow port objects to be linked with line cards
>> >>   netdevsim: create devlink line card object and implement provisioning
>> >>   netdevsim: implement line card activation
>> >>   selftests: add netdevsim devlink lc test
>> >> 
>> >>  drivers/net/netdevsim/bus.c                   |  21 +-
>> >>  drivers/net/netdevsim/dev.c                   | 370 ++++++++++++++-
>> >>  drivers/net/netdevsim/netdev.c                |   2 +
>> >>  drivers/net/netdevsim/netdevsim.h             |  23 +
>> >>  include/net/devlink.h                         |  44 ++
>> >>  include/uapi/linux/devlink.h                  |  25 +
>> >>  net/core/devlink.c                            | 443 +++++++++++++++++-
>> >>  .../drivers/net/netdevsim/devlink.sh          |  62 ++-
>> >>  8 files changed, 964 insertions(+), 26 deletions(-)
>> >> 
>> >> -- 
>> >> 2.26.2
>> >>