Message-ID: <CAKgT0UdT5yPESZd2U=K-n47pDpAjH4Ed08KKrK-Hf9F=N1hm_Q@mail.gmail.com>
Date:   Wed, 29 Aug 2018 10:04:50 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     valex@...lanox.com
Cc:     Saeed Mahameed <saeedm@....mellanox.co.il>,
        Saeed Mahameed <saeedm@...lanox.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...lanox.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Bjorn Helgaas <helgaas@...nel.org>
Subject: Re: [pull request][net-next 00/10] Mellanox, mlx5 and devlink updates 2018-07-31

On Wed, Aug 29, 2018 at 8:43 AM Alex Vesker <valex@...lanox.com> wrote:
>
>
> > On Wed, Aug 1, 2018 at 4:13 PM, Saeed Mahameed
> > <saeedm@....mellanox.co.il> wrote:
> >> On Wed, Aug 1, 2018 at 3:34 PM, Alexander Duyck
> >> <alexander.duyck@...il.com> wrote:
> >>> On Wed, Aug 1, 2018 at 2:52 PM, Saeed Mahameed <saeedm@...lanox.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 ?
> >
> > Either would work for me.
> >
> For which patches are you missing documentation?

Any interfaces that you are exposing via the devlink interface should
have a document somewhere that explains if, when, and how to use it.
All of these new interfaces you were adding in the patch set has no
explanation of what they are other than what is in the driver code
itself and most users don't want to try and decode the driver itself
to figure out what it is they need to do to use an interface.

> >>> 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 ?
> >
> > You have added a vendor specific config section and you are using it
> > to access several of the pieces of metadata. The setup isn't too
> > different than the VPD setup and approach. However I don't see many of
> > the protections that exist for VPD in place for this vendor specific
> > configuration. As such I have concerns. For example what is to keep
> > requests to the various devlink interfaces from racing with each other
> > when they both end up operating through the VCS?
> >
> > - Alex
>
> Hi, I would like to resubmit the devlink region crdump support for mlx5,
> which is part of this patch-set.
>
> AlexD, regarding the protection, various devlink interfaces cannot race
> since
> devlink_mutex is used. The VSC access is also protected, using
> mlx5_vsc_gw_lock/unlock
> only one can acquire the lock.

You are talking about from within the driver right? What about raw PCI
configuration space access? Is there anything to keep setpci from
causing issues when you are using the interface? Really creating a
vendor specific config block in and of itself creates a whole new
mess.

> After explaining this I want to clarify something, the access to VSC is
> not user driven
> it happens automatically by the driver when an error is detected to
> collect a crdump.

You don't prevent a user from being able to poke at the configuration
space via setpci.

Powered by blists - more mailing lists