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: <20210115180145.GA2074023@shredder.lan>
Date:   Fri, 15 Jan 2021 20:01:45 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Jiri Pirko <jiri@...nulli.us>
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

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

> 
> 
> >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.

> 
> >
> >> $ 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
> >>