Message-ID: <BY5PR12MB4322E990BCAF8BD6B2248E72DCF30@BY5PR12MB4322.namprd12.prod.outlook.com>
Date:   Wed, 2 Dec 2020 04:27:30 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jacob.e.keller@...el.com" <jacob.e.keller@...el.com>,
        Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH net-next v2] devlink: Add devlink port documentation



> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, December 2, 2020 7:05 AM
> 
> On Mon, 30 Nov 2020 22:00:25 +0200 Parav Pandit wrote:
> > Added documentation for devlink port and port function related commands.
> >
> > Signed-off-by: Parav Pandit <parav@...dia.com>
> > Reviewed-by: Jiri Pirko <jiri@...dia.com>
> 
> > +============
> > +Devlink Port
> > +============
> > +
> > +``devlink-port`` provides capability for a driver to expose various
> > +flavours of ports which exist on device. A devlink port can be of an
> > +embedded switch (eswitch) present on the device.
> 
> The wording is a little awkward here.
> 
> The first paragraph should clarify what object represents.
> 
> This just says it exposes ports that exist.
> 
> A better phrasing would be to say that these are ports of an eswitch, in trivial
> case the only ports will be the physical ports of the card.
>
Sometimes it is eswitch port, sometimes its physical port on card, sometimes virtual port.
Will rephase.
 
> > +A devlink port can be of 3 diffferent types.
> 
> "can be of" repeated from the previous line
> 
Will rephase.

> > +.. list-table:: List of devlink port types
> > +   :widths: 23 90
> > +
> > +   * - Type
> > +     - Description
> > +   * - ``DEVLINK_PORT_TYPE_ETH``
> > +     - This type is set for a devlink port when a physical link layer
> > + of the port
> 
> Is "physical link layer" a thing? I the common names are physical layer and a
> (data) link layer. I don't think I've seen physical link layer, or would know what it
> is...
I will drop 'physical'. And just say link layer.

> 
> > +       is Ethernet.
> > +   * - ``DEVLINK_PORT_TYPE_IB``
> > +     - This type is set for a devlink port when a physical link layer of the port
> > +       is InfiniBand.
> > +   * - ``DEVLINK_PORT_TYPE_AUTO``
> > +     - This type is indicated by the user when user prefers to set the port type
> > +       to be automatically detected by the device driver.
> 
> IMO type should be after flavor. Flavor is a higher level attribute, only physical
> ports have a type.
> 
I will shift flavour up.
virtual port flavour also has type as eth/ib.

> > +Devlink port can be of few different flavours described below.
> > +
> > +.. list-table:: List of devlink port flavours
> > +   :widths: 33 90
> > +
> > +   * - Flavour
> > +     - Description
> > +   * - ``DEVLINK_PORT_FLAVOUR_PHYSICAL``
> > +     - Any kind of port which is physically facing the user. This can
> > + be
> 
> Hm. Not a great phrasing :(
> 
> It faces a physical networking layer. To me PCIe faces the user.
>
PCIe and physical networking ports, both are visible to users.
Former is usually inside the server, later is more frequently visible outside where networking cable is connected.
 
So, how about wording it as,

Any kind of physical networking port.

> > +       a eswitch physical port or any other physical port on the device.
> > +   * - ``DEVLINK_PORT_FLAVOUR_CPU``
> > +     - This indicates a CPU port.
> 
> You need to mention this is a DSA-only thing.
> 
Ok. will add.

> > +   * - ``DEVLINK_PORT_FLAVOUR_DSA``
> > +     - This indicates a interconnect port in a distributed switch architecture.
> 
> (DSA)
> 
> > +   * - ``DEVLINK_PORT_FLAVOUR_PCI_PF``
> > +     - This indicates an eswitch port representing PCI physical function(PF).
> > +   * - ``DEVLINK_PORT_FLAVOUR_PCI_VF``
> > +     - This indicates an eswitch port representing PCI virtual function(VF).
> > +   * - ``DEVLINK_PORT_FLAVOUR_VIRTUAL``
> > +     - This indicates a virtual port facing the user.
> 
> No idea what that means from the description.
>
Let me rephase it as below.

This indicates a virtual port for the virtual PCI device such as PCI VF.
 
> > +A devlink port may be for a controller consisting one or more PCI device(s).
> 
> Port can have multiple PCI devices?
For each PCI device there is a port as depicted in the diagram from commit 3a2d9588c4f7.

> 
> > +A devlink instance holds ports of two types of controllers.
> > +
> > +(1) controller discovered on same system where eswitch resides This
> > +is the case where PCI PF/VF of a controller and devlink eswitch
> > +instance both are located on a single system.
> > +
> > +(2) controller located on external host system.
> > +This is the case where a controller is located in one system and its
> > +devlink eswitch ports are located in a different system.
> > +
> > +An example view of two controller systems::
> > +
> > +                 ---------------------------------------------------------
> > +                 |                                                       |
> > +                 |           --------- ---------         ------- ------- |
> > +    -----------  |           | vf(s) | | sf(s) |         |vf(s)| |sf(s)| |
> > +    | server  |  | -------   ----/---- ---/----- ------- ---/--- ---/--- |
> > +    | pci rc  |=== | pf0 |______/________/       | pf1 |___/_______/     |
> > +    | connect |  | -------                       -------                 |
> > +    -----------  |     | controller_num=1 (no eswitch)                   |
> > +                 ------|--------------------------------------------------
> > +                 (internal wire)
> > +                       |
> > +                 ---------------------------------------------------------
> > +                 | devlink eswitch ports and reps                        |
> > +                 | ----------------------------------------------------- |
> > +                 | |ctrl-0 | ctrl-0 | ctrl-0 | ctrl-0 | ctrl-0 |ctrl-0 | |
> > +                 | |pf0    | pf0vfN | pf0sfN | pf1    | pf1vfN |pf1sfN | |
> > +                 | ----------------------------------------------------- |
> > +                 | |ctrl-1 | ctrl-1 | ctrl-1 | ctrl-1 | ctrl-1 |ctrl-1 | |
> > +                 | |pf0    | pf0vfN | pf0sfN | pf1    | pf1vfN |pf1sfN | |
> > +                 | ----------------------------------------------------- |
> > +                 |                                                       |
> > +                 |                                                       |
> > +                 |           --------- ---------         ------- ------- |
> > +                 |           | vf(s) | | sf(s) |         |vf(s)| |sf(s)| |
> > +                 | -------   ----/---- ---/----- ------- ---/--- ---/--- |
> > +                 | | pf0 |______/________/       | pf1 |___/_______/     |
> > +                 | -------                       -------                 |
> > +                 |                                                       |
> > +                 |  local controller_num=0 (eswitch)                     |
> > +
> > + ---------------------------------------------------------
> > +
> > +Port function configuration
> > +===========================
> > +
> > +When a port flavor is ``DEVLINK_PORT_FLAVOUR_PCI_PF`` or
> > +``DEVLINK_PORT_FLAVOUR_PCI_VF``, it represents the port of a PCI function.
> > +A user can configure the port function attributes before enumerating
> > +the function. For example user may set the hardware address of the
> > +function represented by the devlink port.
> > diff --git a/Documentation/networking/devlink/index.rst
> > b/Documentation/networking/devlink/index.rst
> > index d82874760ae2..aab79667f97b 100644
> > --- a/Documentation/networking/devlink/index.rst
> > +++ b/Documentation/networking/devlink/index.rst
> > @@ -18,6 +18,7 @@ general.
> >     devlink-info
> >     devlink-flash
> >     devlink-params
> > +   devlink-port
> >     devlink-region
> >     devlink-resource
> >     devlink-reload

Powered by blists - more mailing lists