Re: [pull request][net-next 00/10] Mellanox, mlx5 and devlink updates 2018-07-31

Saeed Mahameed
2018-08-02T01:01:52Z

On Wed, Aug 1, 2018 at 3:34 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Aug 1, 2018 at 2:52 PM, Saeed Mahameed <saeedm@mellanox.com> wrote:
>> Hi Dave,
>>
>> This series provides devlink parameters updates to both devlink API and
>> mlx5 driver, it is a 2nd iteration of the dropped patches sent in a previous
>> mlx5 submission "net/mlx5: Support PCIe buffer congestion handling via
>> Devlink" to address review comments [1].
>>
>> Changes from the original series:
>> - According to the discussion outcome, we are keeping the congestion control
>> setting as mlx5 device specific for the current HW generation.
>> - Changed the congestion_mode and congestion action param type to string
>> - Added patches to fix devlink handling of param type string
>> - Added a patch which adds extack messages support for param set.
>> - At the end of this series, I've added yet another mlx5 devlink related
>> feature, firmware snapshot support.
>>
>> For more information please see tag log below.
>>
>> Please pull and let me know if there's any problem.
>>
>> [1] https://patchwork.ozlabs.org/patch/945996/
>>
>> Thanks,
>> Saeed.
>>
>> ---
>>
>> The following changes since commit e6476c21447c4b17c47e476aade6facf050f31e8:
>>
>> net: remove bogus RCU annotations on socket.wq (2018-07-31 12:40:22 -0700)
>>
>> are available in the Git repository at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-updates-2018-08-01
>>
>> for you to fetch changes up to 2ac6108c65ffcb1e5eab1fba1fd59272604d1c32:
>>
>> net/mlx5: Use devlink region_snapshot parameter (2018-08-01 14:49:09 -0700)
>>
>> ----------------------------------------------------------------
>> mlx5-updates-2018-08-01
>>
>> This series provides devlink parameters updates to both devlink API and
>> mlx5 driver,
>>
>> 1) Devlink changes: (Moshe Shemesh)
>> The first two patches fix devlink param infrastructure for string type
>> params.
>> The third patch adds a devlink helper function to safely copy string from
>> driver to devlink.
>> The forth patch adds extack support for param set.
>>
>> 2) mlx5 specific congestion parameters: (Eran Ben Elisha)
>> Next three patches add new devlink driver specific params for controlling
>> congestion action and mode, using string type params and extack messages support.
>>
>> This congestion mode enables hw workaround in specific devices which is
>> controlled by devlink driver-specific params. The workaround is device
>> specific for this NIC generation, the next NIC will not need it.
>>
>> Congestion parameters:
>> - Congestion action
>> HW W/A mechanism in the PCIe buffer which monitors the amount of
>> consumed PCIe buffer per host. This mechanism supports the
>> following actions in case of threshold overflow:
>> - Disabled - NOP (Default)
>> - Drop
>> - Mark - Mark CE bit in the CQE of received packet
>> - Congestion mode
>> - Aggressive - Aggressive static trigger threshold (Default)
>> - Dynamic - Dynamically change the trigger threshold
>>
>> 3) mlx5 firmware snapshot support via devlink: (Alex Vesker)
>> Last three patches, add the support for capturing region snapshot of the
>> firmware crspace during critical errors, using devlink region_snapshot
>> parameter.
>>
>> -Saeed.
>>
>> ----------------------------------------------------------------
>> Alex Vesker (3):
>> net/mlx5: Add Vendor Specific Capability access gateway
>> net/mlx5: Add Crdump FW snapshot support
>> net/mlx5: Use devlink region_snapshot parameter
>>
>> Eran Ben Elisha (3):
>> net/mlx5: Move all devlink related functions calls to devlink.c
>> net/mlx5: Add MPEGC register configuration functionality
>> net/mlx5: Enable PCIe buffer congestion handling workaround via devlink
>>
>> Moshe Shemesh (4):
>> devlink: Fix param set handling for string type
>> devlink: Fix param cmode driverinit for string type
>> devlink: Add helper function for safely copy string param
>> devlink: Add extack messages support to param set
>>
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 3 +-
>> drivers/net/ethernet/mellanox/mlx4/main.c | 6 +-
>> drivers/net/ethernet/mellanox/mlx5/core/Makefile | 3 +-
>> drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 388 +++++++++++++++++++++
>> drivers/net/ethernet/mellanox/mlx5/core/devlink.h | 13 +
>> .../net/ethernet/mellanox/mlx5/core/diag/crdump.c | 223 ++++++++++++
>> drivers/net/ethernet/mellanox/mlx5/core/health.c | 3 +
>> drivers/net/ethernet/mellanox/mlx5/core/lib/mlx5.h | 4 +
>> .../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c | 320 +++++++++++++++++
>> .../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h | 56 +++
>> drivers/net/ethernet/mellanox/mlx5/core/main.c | 10 +-
>> include/linux/mlx5/driver.h | 5 +
>> include/net/devlink.h | 15 +-
>> net/core/devlink.c | 44 ++-
>> 14 files changed, 1076 insertions(+), 17 deletions(-)
>> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.c
>> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.h
>> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/diag/crdump.c
>> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c
>> create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h
>
> So after looking over the patch set the one thing I would ask for in
> this is some sort of documentation at a minimum. As a user I don't see
> how you can expect someone to be able to use this when the naming of
> things are pretty cryptic and there is no real explanation anywhere if
> you don't go through and read the patch description itself. When you
> start adding driver specific interfaces, you should at least start
> adding vendor specific documentation.
>

Sure, sounds like a great idea, something like:
Documentation/networking/mlx5.txt and have a devlink section ?
or have a generic devlink doc and a mlx5 section in it ?

> Also I don't see how using a vendor specific configuration space
> section can be done without adding some tie-ins to the PCI core files
> because it should be possible to race with someone poking at the
> register space via something like setpci/lspci. Also one of the things
> that came up was that drivers are not supposed to be banging on the
> PCI configuration space at will, and it seems like this patch set is
> doing exactly that through the VSC block.
>

this is a whole different feature than the device specific parameters.
The whole vendor specific configuration space access is needed only
for diagnostic/dump
purposes when something really bad happens and the command interface
with FW is down,
and when the FW is un-responsive, we want to dump the crspace into the
already existing devlink
crdump buffer, how do you expect us to read it if we are not allowed
to access it ?

What do you mean by tie-ins to the PCI core files ? can you please elaborate ?

> - Alex