Date:   Mon, 2 Mar 2020 17:27:58 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org, valex@...lanox.com, linyunsheng@...wei.com,
        lihong.yang@...el.com, kuba@...nel.org
Subject: Re: [RFC PATCH v2 00/22] devlink region updates

Sat, Feb 15, 2020 at 12:21:59AM CET, jacob.e.keller@...el.com wrote:
>This is a second revision of the previous RFC series I sent to enable two
>new devlink region features.
>
>The original series can be viewed on the list archives at
>
>https://lore.kernel.org/netdev/20200130225913.1671982-1-jacob.e.keller@intel.com/
>
>Overall, this series can be broken into 5 phases:
>
> 1) implement basic devlink support in the ice driver, including .info_get
> 2) convert regions to use the new devlink_region_ops structure
> 3) implement support for DEVLINK_CMD_REGION_NEW
> 4) implement support for directly reading from a region
> 5) use these new features in the ice driver for the Shadow RAM region

Hmm. I think it is better to push this in multiple patchsets. For example,
for 1) you don't really need RFC as it is only related to the ice driver
implementing the existing API.


>
>(1) comprises 6 patches for the ice driver that add the devlink framework
>and cleanup a few places in the code in preparation for the new region.
>
>(2) comprises 2 patches which convert regions to use the new
>devlink_region_ops structure, and additionally move the snapshot destructor
>to a region operation.
>
>(3) comprises 6 patches to enable supporting the DEVLINK_CMD_REGION_NEW
>operation. This replaces what was previously the
>DEVLINK_CMD_REGION_TAKE_SNAPSHOT, as per Jiri's suggestion. The new
>operation supports specifying the requested id for the snapshot. To make
>that possible, first snapshot id management is refactored to use an IDR.
>Note that the extra complexity of the IDR is necessary in order to maintain
>the ability for the snapshot IDs to be generated so that multiple regions
>can use the same ID if triggered at the same time.
>
>(4) comprises 6 patches for modifying DEVLINK_CMD_REGION_READ so that it
>accepts a request without a snapshot id. A new region operation is defined
>for regions to optionally support the requests. The first few patches
>refactor and simplify the functions used so that adding the new read method
>reuses logic where possible.
>
>(5) finally comprises a single patch to implement a region for the ice
>device hardware's Shadow RAM contents.
>
>Note that I plan to submit the ice patches through the Intel Wired LAN list,
>but am sending the complete set here as an RFC in case there is further
>feedback, and so that reviewers can have the correct context.
>
>I expect to get further feedback this RFC revision, and will hopefully send
>the patches as non-RFC following this, if feedback looks good. Thank you for
>the diligent review.
>
>Changes since v1:

Per-patch please. This is no good for review :/


>
>* reword some comments and variable names in the ice driver that used the
>  term "page" to use the term "sector" to avoid confusion with the PAGE_SIZE
>  of the system.
>* Fixed a bug in the ice_read_flat_nvm function due to misusing the last_cmd
>  variable
>* Remove the devlinkm* functions and just use devm_add_action in the ice
>  driver for managing the devlink memory similar to how the PF memory was
>  managed by the devm_kzalloc.
>* Fix typos in a couple of function comments in ice_devlink.c
>* use dev_err instead of dev_warn for an error case where the main VSI can't
>  be found.
>* Only call devlink_port_type_eth_set if the VSI has a netdev
>* Move where the devlink_port is created in the ice_probe flow
>* Update the new ice.rst documentation for info versions, providing more
>  clear descriptions of the parameters. Give examples for each field as
>  well. Squash the documentation additions into the relevant patches.
>* Add a new patch to the ice driver which renames some variables referring
>  to the Option ROM version.
>* keep the string constants in the mlx4 crdump.c file, converting them to
>  "const char * const" so that the compiler understands they can be used in
>  constant initializers.
>* Add a patch to convert snapshot destructors into a region operation
>* Add a patch to fix a trivial typo in a devlink function comment
>* Use __ as a prefix for static internal functions instead of a _locked
>  suffix.
>* Refactor snapshot id management to use an IDR.
>* Implement DEVLINK_CMD_REGION_NEW of DEVLINK_CMD_REGION_TAKE_SNAPSHOT
>* Add several patches which refactor devlink_nl_cmd_region_snapshot_fill
>* Use the new cb_ and cb_priv parameters to implement what was previously
>  a separate function called devlink_nl_cmd_region_direct_fill
>
>Jacob Keller (21):
>  ice: use __le16 types for explicitly Little Endian values
>  ice: create function to read a section of the NVM and Shadow RAM
>  ice: enable initial devlink support
>  ice: rename variables used for Option ROM version
>  ice: add basic handler for devlink .info_get
>  ice: add board identifier info to devlink .info_get
>  devlink: prepare to support region operations
>  devlink: convert snapshot destructor callback to region op
>  devlink: trivial: fix tab in function documentation
>  devlink: add functions to take snapshot while locked
>  devlink: convert snapshot id getter to return an error
>  devlink: track snapshot ids using an IDR and refcounts
>  devlink: implement DEVLINK_CMD_REGION_NEW
>  netdevsim: support taking immediate snapshot via devlink
>  devlink: simplify arguments for read_snapshot_fill
>  devlink: use min_t to calculate data_size
>  devlink: report extended error message in region_read_dumpit
>  devlink: remove unnecessary parameter from chunk_fill function
>  devlink: refactor region_read_snapshot_fill to use a callback function
>  devlink: support directly reading from region memory
>  ice: add a devlink region to dump shadow RAM contents
>
>Jesse Brandeburg (1):
>  ice: implement full NVM read from ETHTOOL_GEEPROM
>
> .../networking/devlink/devlink-region.rst     |  20 +-
> Documentation/networking/devlink/ice.rst      |  87 ++++
> Documentation/networking/devlink/index.rst    |   1 +
> drivers/net/ethernet/intel/Kconfig            |   1 +
> drivers/net/ethernet/intel/ice/Makefile       |   1 +
> drivers/net/ethernet/intel/ice/ice.h          |   6 +
> .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   3 +
> drivers/net/ethernet/intel/ice/ice_common.c   |  85 +---
> drivers/net/ethernet/intel/ice/ice_common.h   |  10 +-
> drivers/net/ethernet/intel/ice/ice_devlink.c  | 360 ++++++++++++++
> drivers/net/ethernet/intel/ice/ice_devlink.h  |  17 +
> drivers/net/ethernet/intel/ice/ice_ethtool.c  |  44 +-
> drivers/net/ethernet/intel/ice/ice_main.c     |  23 +-
> drivers/net/ethernet/intel/ice/ice_nvm.c      | 354 +++++++------
> drivers/net/ethernet/intel/ice/ice_nvm.h      |  12 +
> drivers/net/ethernet/intel/ice/ice_type.h     |  17 +-
> drivers/net/ethernet/mellanox/mlx4/crdump.c   |  32 +-
> drivers/net/netdevsim/dev.c                   |  41 +-
> include/net/devlink.h                         |  38 +-
> net/core/devlink.c                            | 465 ++++++++++++++----
> .../drivers/net/netdevsim/devlink.sh          |  15 +
> 21 files changed, 1257 insertions(+), 375 deletions(-)
> create mode 100644 Documentation/networking/devlink/ice.rst
> create mode 100644 drivers/net/ethernet/intel/ice/ice_devlink.c
> create mode 100644 drivers/net/ethernet/intel/ice/ice_devlink.h
>
>-- 
>2.25.0.368.g28a2d05eebfb
>

Powered by blists - more mailing lists