Message-ID: <20200914114318.GD2236@nanopsycho.orion>
Date:   Mon, 14 Sep 2020 13:43:18 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Moshe Shemesh <moshe@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next RFC v4 15/15] devlink: Add
 Documentation/networking/devlink/devlink-reload.rst

Mon, Sep 14, 2020 at 08:08:02AM CEST, moshe@...lanox.com wrote:
>Add devlink reload rst documentation file.
>Update index file to include it.
>
>Signed-off-by: Moshe Shemesh <moshe@...lanox.com>
>---
>v3 -> v4:
>- Remove reload action fw_activate_no_reset
>- Add reload actions limit levels and document the no_reset limit level
>  constrains
>v2 -> v3:
>- Devlink reload returns the actions done
>- Replace fw_live_patch action by fw_activate_no_reset
>- Explain fw_activate meaning
>v1 -> v2:
>- Instead of reload levels driver,fw_reset,fw_live_patch have reload
>  actions driver_reinit,fw_activate,fw_live_patch
>---
> .../networking/devlink/devlink-reload.rst     | 80 +++++++++++++++++++
> Documentation/networking/devlink/index.rst    |  1 +
> 2 files changed, 81 insertions(+)
> create mode 100644 Documentation/networking/devlink/devlink-reload.rst
>
>diff --git a/Documentation/networking/devlink/devlink-reload.rst b/Documentation/networking/devlink/devlink-reload.rst
>new file mode 100644
>index 000000000000..6ac9dddd2208
>--- /dev/null
>+++ b/Documentation/networking/devlink/devlink-reload.rst
>@@ -0,0 +1,80 @@
>+.. SPDX-License-Identifier: GPL-2.0
>+
>+==============
>+Devlink Reload
>+==============
>+
>+``devlink-reload`` provides mechanism to either reload driver entities,
>+applying ``devlink-params`` and ``devlink-resources`` new values or firmware
>+activation depends on reload action selected.
>+
>+Reload actions
>+==============
>+
>+User may select a reload action.
>+By default ``driver_reinit`` action is selected.
>+
>+.. list-table:: Possible reload actions
>+   :widths: 5 90
>+
>+   * - Name
>+     - Description
>+   * - ``driver-reinit``
>+     - Devlink driver entities re-initialization, including applying
>+       new values to devlink entities which are used during driver
>+       load such as ``devlink-params`` in configuration mode
>+       ``driverinit`` or ``devlink-resources``
>+   * - ``fw_activate``
>+     - Firmware activate. Activates new firmware if such image is stored and
>+       pending activation. This action involves firmware reset, if no new image
>+       pending this action will reload current firmware image.
>+
>+Note that when required to do firmware activation some drivers may need
>+to reload the driver. On the other hand some drivers may need to reset

s/reload/reinit" ?

>+the firmware to reinitialize the driver entities. Therefore, the devlink
>+reload command returns the actions which were actually performed.

I would perhaps say something more generic like:
Note that even though user asks for a specific action, the driver
implementation might require to perform another action alongside with
it. For example, some driver do not support driver reinitialization
being performed without fw activation. Therefore, the devlink reload
command return the list of actions which were actrually performed.


>+
>+Reload action limit levels
>+==========================
>+
>+By default reload actions are not limited and Driver implementation may

Why capital "D"?


>+include reset or downtime as needed to perform the actions.
>+
>+However, some drivers support action limit levels, which limits the action
>+implementation to specific constrains.
>+
>+.. list-table:: Possible reload action limit levels
>+   :widths: 5 90
>+
>+   * - Name
>+     - Description
>+   * - ``no_reset``
>+     - No reset allowed, no down time allowed, no link flap and no
>+       configuration is lost.
>+
>+Change namespace
>+================
>+
>+All devlink instances are created in init_net and stay there for a
>+lifetime. Allow user to be able to move devlink instances into
>+namespaces during devlink reload operation. That ensures proper
>+re-instantiation of driver objects, including netdevices.

This sounds like a commit message :) Could you please re-phrase a bit?


>+
>+example usage
>+-------------
>+
>+.. code:: shell
>+
>+    $ devlink dev reload help
>+    $ devlink dev reload DEV [ netns { PID | NAME | ID } ] [ action { driver_reinit | fw_activate } ] [limit_level no_reset]
>+
>+    # Run reload command for devlink driver entities re-initialization:
>+    $ devlink dev reload pci/0000:82:00.0 action driver_reinit
>+    reload_actions_performed:
>+      driver_reinit
>+
>+    # Run reload command to activate firmware:
>+    # Note that mlx5 driver reloads the driver while activating firmware
>+    $ devlink dev reload pci/0000:82:00.0 action fw_activate
>+    reload_actions_performed:
>+      driver_reinit fw_activate

This looks fine to me.


>diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
>index 7684ae5c4a4a..d82874760ae2 100644
>--- a/Documentation/networking/devlink/index.rst
>+++ b/Documentation/networking/devlink/index.rst
>@@ -20,6 +20,7 @@ general.
>    devlink-params
>    devlink-region
>    devlink-resource
>+   devlink-reload
>    devlink-trap
> 
> Driver-specific documentation
>-- 
>2.17.1
>

Powered by blists - more mailing lists