Message-ID: <BY5PR12MB43227128D9DEDC9E41744017DCCE0@BY5PR12MB4322.namprd12.prod.outlook.com>
Date:   Mon, 7 Dec 2020 04:46:14 +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 v4] devlink: Add devlink port documentation



> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Sunday, December 6, 2020 1:57 AM
> 
> On Thu, 3 Dec 2020 20:02:55 +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>
> > Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> 
> > +============
> > +Devlink Port
> > +============
> > +
> > +``devlink-port`` is a port that exists on the device.
> 
> Can we add something like:
> 
> Each port is a logically separate ingress/egress point of the device.
> 
> ?
This may not be true when both physical ports are under bond.

> 
> > A devlink port can
> > +be of one among many flavours. A devlink port flavour along with port
> > +attributes describe what a port represents.
> > +
> > +A device driver that intends to publish a devlink port sets the
> > +devlink port attributes and registers the devlink port.
> > +
> > +Devlink port types are described below.
> 
> How about:
> 
> Physical ports can have different types based on the link layer:
>
Ok. will add.
 
> > +.. list-table:: List of devlink port types
> > +   :widths: 23 90
> > +
> > +   * - Type
> > +     - Description
> > +   * - ``DEVLINK_PORT_TYPE_ETH``
> > +     - Driver should set this port type when a link layer of the port is Ethernet.
> > +   * - ``DEVLINK_PORT_TYPE_IB``
> > +     - Driver should set this port type when a link layer of the port is InfiniBand.
> 
> Please wrap at 80 chars.
> 
Ack.
> > +   * - ``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.
> 
> How about:
> 
> This type is indicated by the user when driver should detect the port type
> automatically.
> 
Will change.

> > +A controller consists of one or more PCI functions.
> 
> This need some intro. Like:
> 
> PCI controllers
> ---------------
> 
> In most cases PCI devices will have only one controller, with potentially multiple
> physical and virtual functions. Devices connected to multiple CPUs and
> SmartNICs, however, may have multiple controllers.
> 
> > Such PCI function consists
> > +of one or more networking ports.
> 
> PCI function consists of networking ports? What do you mean by a networking
> port? All devlink ports are networking ports.
>
I am not sure this document should be a starting point to define such restriction.
 
> > A networking port of such PCI function is
> > +represented by the eswitch devlink port.
> 
> What's eswitch devlink port? It was never defined.
Eswitch devlink port is the port which sets eswitch attributes (id and length).

> 
> > A devlink instance holds ports of two
> > +types of controllers.
> 
> For devices with multiple controllers we can distinguish...
> 
Yes, will change.

> > +(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.
> 
> How is eswitch located on a system? Eswitch is in the NIC
>
Yes, I meant eswitch devlink instance and controller devlink instance are same.
 Will rephase.

> I think you should say refer to eswitch being controlled by a system.
> 
> > +(2) controller located on external host system.
> > +This is the case where a controller is in one system and its devlink
> > +eswitch ports are in a different system. Such controller is called
> > +external controller.
> 
> > +An example view of two controller systems::
> > +
> > +Port function configuration
> > +===========================
> > +
> > +When a port flavor is ``DEVLINK_PORT_FLAVOUR_PCI_PF`` or
> > +``DEVLINK_PORT_FLAVOUR_PCI_VF``, it represents the networking port of
> > +a PCI function.
> 
> Networking port of a PCI function?
> 
> > A user can configure the port function attributes
> 
> port function attributes?
> 
> > before
> > +enumerating the function.
> 
> What does this mean? What does enumerate mean in this context?
> 
Enumerate means before creating the device of the function.
However today due to SR-IOV limitation, it is before probing the function device.

> > For example user may set the hardware address of
> > +the function represented by the devlink port function.
> 
> What's a hardware address? You mean MAC address?
Yes, MAC address.
Port function attribute is named as hardware address to be generic enough similar to other iproute2 tools.

Powered by blists - more mailing lists