Re: [dpdk-dev] [PATCH v2 04/11] doc: Add BNX2X PMD documentation

["Mcnamara, John" <john.mcnamara@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rasesh Mody
> Sent: Monday, November 9, 2015 11:56 PM
> To: thomas.monjalon@6wind.com
> Cc: dev@dpdk.org; sony.chacko@qlogic.com
> Subject: [dpdk-dev] [PATCH v2 04/11] doc: Add BNX2X PMD documentation
>
> From: Harish Patil <harish.patil@qlogic.com>



Hi,

Thanks for the new PMD and the documentation.

The DPDK documentation guidelines provide some guidance on how to format and
build the DPDK documentation:

    http://dpdk.org/doc/guides/contributing/documentation.html

In terms of documentation for a PMD the Mellanox MLX4 docs are a good example:

    http://dpdk.org/doc/guides/nics/mlx4.html

Note, the bnx2x doc needs to be added to doc/guides/nics/index.rst. 

There are also the following build warnings:

    $ make doc-guides-html
    sphinx processing guides-html...

    doc/guides/nics/bnx2x.rst:68:
    WARNING: Bullet list ends without a blank line; unexpected unindent.

    doc/guides/nics/bnx2x.rst:73:
    WARNING: Bullet list ends without a blank line; unexpected unindent.

    doc/guides/nics/bnx2x.rst:321:
    WARNING: duplicate label freebsd-installation, other instance
             in doc/guides/nics/cxgbe.rst

    doc/guides/nics/bnx2x.rst:126:
    WARNING: duplicate label driver-compilation, other instance
             in doc/guides/nics/cxgbe.rst

    doc/guides/nics/bnx2x.rst:158:
    WARNING: duplicate label linux-installation, other instance
             in doc/guides/nics/cxgbe.rst

    doc/guides/nics/bnx2x.rst::
    WARNING: document isn't included in any toctree


Some other comments below:


> +BNX2X Poll Mode Driver
> +======================
> +
> +The BNX2X poll mode driver library (**librte_pmd_bnx2x**) implemements

Implements (check other spelling as well).


> +Co-existence considerations
> +---------------------------
> +
> +- BCM578xx being a CNA can have both NIC and Storage personalities.
> +However, coexistence with storage protocol drivers (cnic, bnx2fc and
> +bnx2fi) is not supported on the same adapter. So storage personality
> +has to be disabled on that adapter when used in DPDK applications.
> +
> +- For SR-IOV case, bnx2x PMD will be used to bind to SR-IOV VF device
> +and linux native kernel driver (bnx2x) will be attached to SR-IOV PF.


The follow-on sentences must be indented to the same level as the bullet
marker, like this:

- BCM578xx being a CNA can have both NIC and Storage personalities.
  However, coexistence with storage protocol drivers (cnic, bnx2fc and
  bnx2fi) is not supported on the same adapter. So storage personality
  has to be disabled on that adapter when used in DPDK applications.


> +- Requires firmware version **7.2.51.0**. It is inbox on most of the
> +  standard linux distros. If it is not available visit
> +  `QLogic Driver Download Center <http://driverdownloads.qlogic.com>`


Missing _ at the end of the link formatting:

    ... <http://driverdownloads.qlogic.com>`_.


> +.. _driver-compilation:

This, and other labels in the doc, aren't unique to the total documentation
and generate warnings.

Either, omit them (I don't see them used) or make them unique by prefixing
the doc name:

    .. _bnx2x_driver_compilation:



> +
> +#. Bind the QLogic adapters to igb_uio or vfio-pci loaded in the previous

It is worth using fixed width font formatting for things like application or
component names:

    #. Load ``igb_uio`` or ``vfio-pci`` driver:

This could be used in other places as well.



> +   step:
> +
> +   .. code-block:: console
> +
> +      ./tools/dpdk_nic_bind.py --bind igb_uio 0000:84:00.0 0000:84:00.1


If you don't need an explicit console block (usually only required for console
input or output) then you can use the simpler format of "::" plus indentation:

      step::

          ./tools/dpdk_nic_bind.py --bind igb_uio 0000:84:00.0 0000:84:00.1


> +      Interactive-mode selected
> +      Configuring Port 0 (socket 0)
> +      PMD: bnx2x_dev_tx_queue_setup(): fp[00] req_bd=512, thresh=512,
> usable_bd=1020, total_bd=1024, tx_pages=4
> +      PMD: bnx2x_dev_rx_queue_setup(): fp[00] req_bd=128, thresh=0,
> usable_bd=510, total_bd=512, rx_pages=1, cq_pages=8
> +      PMD: bnx2x_print_adapter_info():

Lines longer than 80 chars in verbatim/code blocks should be avoided since
they don't render correctly in pdf. The DPDK documentation guidelines
recommend wrapping the lines where possible. The following gives the
same information:


      Configuring Port 0 (socket 0)
      PMD: bnx2x_dev_tx_queue_setup(): fp[00] req_bd=512, thresh=512,
                                       usable_bd=1020, total_bd=1024,
				       tx_pages=4
      PMD: bnx2x_dev_rx_queue_setup(): fp[00] req_bd=128, thresh=0,
                                       usable_bd=510, total_bd=512,
                                       rx_pages=1, cq_pages=8
      PMD: bnx2x_print_adapter_info():



> +
> +#. Create VF device(s):
> +
> +   .. code-block:: console
> +
> +      Echo the number of VFs to be created into "sriov_numvfs" sysfs
> entry
> +      of the parent PF.

This is a paragraph and shouldn't be formatted as a console block. There
are 3-4 examples like this that should be fixed in this section.


> +#. PCI passthrough:
> +
> +   .. code-block:: console
> +
> +      The VF devices may be passthru'ed to the guest VM using virt-
> manager or virsh etc.

Probably better as: passed through



> +      bnx2x PMD should be used to bind the VF devices in the guest VM
> using the instructions
> +      outlined in Application notes.

Probably better as: outlined in the Application notes below.


> +FreeBSD
> +-------
> +
> +.. _freebsd-installation:
> +
> +FreeBSD Installation
> +~~~~~~~~~~~~~~~~~~~~
> +
> +Sample Application Notes
> +~~~~~~~~~~~~~~~~~~~~~~~~


It looks like there is some text missing here. If it is in another patch
it should be moved here.

Thanks,

John.
--