Re: [PATCH v4] net/ice: support customized search path for DDP package

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Sep 13, 2024 at 02:15:37PM +0800, Zhichao Zeng wrote:
> This patch adds support for customizing firmware search path for
> DDP package like the kernel behavior, it will read the search path
> from "/sys/module/firmware_class/parameters/path", and try to load
> DDP package.
> 
> Also, updates documentation for loading the DDP package in ice.rst.
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> ---
> v4: fix CI error
> v3: update doc, fix code error
> v2: separate the patch and rewrite the log
> ---
>  doc/guides/nics/ice.rst      | 63 +++++++++++++++++++-----------------
>  drivers/net/ice/ice_ethdev.c | 44 +++++++++++++++++++++++++
>  drivers/net/ice/ice_ethdev.h |  1 +
>  3 files changed, 79 insertions(+), 29 deletions(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..7f347684e7 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -80,6 +80,40 @@ are listed in the Tested Platforms section of the Release Notes for each release
>     |    24.03  |     1.13.7    |      1.3.35     |  1.3.45   |    1.3.13    |    4.4    |
>     +-----------+---------------+-----------------+-----------+--------------+-----------+
>  
> +Dynamic Device Personalization (DDP) package loading
> +----------------------------------------------------
> +
> +The Intel E810 requires a programmable pipeline package be downloaded
> +by the driver to support normal operations. The E810 has a limited
> +functionality built in to allow PXE boot and other use cases, but the
> +driver must download a package file during the driver initialization
> +stage.

Hi Zhichao,

I realise that this text above is not written by you, but when moving this
text we can update it a little.

* in second sentence, drop the word "a" before "limited functionality".
* I feel we are missing a few words in the second sentence to clarify the
  need for DDP support. For example, after the word "but" we could clarify
  with "for normal use" or "for DPDK use" or some similar phrase.

> +
> +The default DDP package file name is ice.pkg. For a specific NIC, the
> +DDP package supposed to be loaded can have a filename: ice-xxxxxx.pkg,

For the filenames: ice.pkg and ice-xxxxxx.pkg, put them in `` quotes to
make them monospaced/literal text. Same for filenames below.

> +where 'xxxxxx' is the 64-bit PCIe Device Serial Number of the NIC. For
> +example, if the NIC's device serial number is 00-CC-BB-FF-FF-AA-05-68,
> +the device-specific DDP package filename is ice-00ccbbffffaa0568.pkg
> +(in hex and all low case), please review README from
> +`Intel® Ethernet 800 Series Dynamic Device Personalization (DDP) for Telecommunication (Comms) Package
> +<https://www.intel.com/content/www/us/en/download/19660/intel-ethernet-800-series-dynamic-device-personalization-ddp-for-telecommunication-comms-package.html>`_
> +for more information. A symbolic link to the DDP package file is also ok.
> +The same package file is used by both the kernel driver and the ICE PMD.
> +
> +ICE PMD support customized DDP search path, driver will read search path

"support" -> "supports using a"
", driver" -> ". The driver"
"search path" -> "the search path"

> +from '/sys/module/firmware_class/parameters/path' as CUSTOMIZED_PATH.
> +During initialization, the driver searches in the following paths in order:
> +CUSTOMIZED_PATH(mentioned above), /lib/firmware/updates/intel/ice/ddp and

you can drop "(mentioned above)".

> +/lib/firmware/intel/ice/ddp. The corresponding device-specific DDP
> +package will be downloaded first if the file exists. If not, then the
> +driver tries to load the default package. The type of loaded package
> +is stored in ``ice_adapter->active_pkg_type``.

The behaviour is not exactly clear here. The text seems to imply (at least
ot me) that the search order is to check each directory first for the
serial-number package and then for ice.pkg, before moving on to the next
directory. However, the actual search pattern in the code  is to check each
directory in turn for a serial number package, and then scan each directory
in turn for an ice.pkg file. Consider the case of having two files:

$CUSTOMIZED_PATH/ice.pkg
/lib/firmware/intel/ice/ddp/ice-xxxxxxx.pkg

As described in the doc text above, I would expect the first of these two
packages to be used, since it's in the customized path, but in fact the
second will be used by the code since it has a serial number on it so is
found in the initial scan for a numbered-package.

> +
> +   .. Note::
> +
> +      Windows support: The DDP package is not supported on Windows so,
> +      loading of the package is disabled on Windows.

I notice that the comma "," is in the wrong place in
the original text - it should come before the "so" not after it - but
overall the whole line can just be shortened to:
	"Windows support: DDP packages are not supported on Windows"

<snip for brevity>

Thanks,
/Bruce