Re: [RFC PATCH v2 00/22] devlink region updates

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3073 bytes --]



On 3/2/2020 8:27 AM, Jiri Pirko wrote:
> Sat, Feb 15, 2020 at 12:21:59AM CET, jacob.e.keller@intel.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.
> 

Yes that's my plan for the next revision. I'm working on getting the ice
support ready to submit through IWL now. The other parts I will break
into 2 series.

> 
>>
>> (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 :/
> 

I've attached the git range-diff between the v1 and v2 series. I'll keep
in mind for future revision logs.

Thanks,
Jake

[-- Attachment #2: range-diff-since-v1.diff --]
[-- Type: text/plain, Size: 29442 bytes --]

 5:  dfe3f13dc7c8 =  1:  3289b0e46c1f ice: use __le16 types for explicitly Little Endian values
 6:  efd2a78e8fb6 !  2:  e702c773bf81 ice: create function to read a section of the NVM and Shadow RAM
    @@ drivers/net/ethernet/intel/ice/ice_nvm.c: ice_aq_read_nvm(struct ice_hw *hw, u16
     + * @data: buffer to return data in (sized to fit the specified length)
     + * @read_shadow_ram: if true, read from shadow RAM instead of NVM
     + *
    -+ * Reads a portion of the NVM, as a flat memory space. This function will
    -+ * correctly handle reading of sizes beyond a page by breaking the request
    -+ * into multiple reads.
    ++ * Reads a portion of the NVM, as a flat memory space. This function correctly
    ++ * breaks read requests across Shadow RAM sectors and ensures that no single
    ++ * read request exceeds the maximum 4Kb read for a single AdminQ command.
     + *
     + * Returns a status code on failure. Note that the data pointer may be
     + * partially updated if some reads succeed before a failure.
    @@ drivers/net/ethernet/intel/ice/ice_nvm.c: ice_aq_read_nvm(struct ice_hw *hw, u16
     +		  bool read_shadow_ram)
     +{
     +	enum ice_status status;
    -+	bool last_cmd = true;
     +	u32 inlen = *length;
     +	u32 bytes_read = 0;
    ++	bool last_cmd;
     +
     +	*length = 0;
     +
     +	/* Verify the length of the read if this is for the Shadow RAM */
    -+	if (read_shadow_ram && ((offset + inlen) > (hw->nvm.sr_words * 2))) {
    ++	if (read_shadow_ram && ((offset + inlen) > (hw->nvm.sr_words * 2u))) {
     +		ice_debug(hw, ICE_DBG_NVM,
     +			  "NVM error: requested offset is beyond Shadow RAM limit\n");
     +		return ICE_ERR_PARAM;
     +	}
     +
     +	do {
    -+		u32 read_size, page_offset;
    ++		u32 read_size, sector_offset;
     +
     +		/* ice_aq_read_nvm cannot read more than 4Kb at a time.
    -+		 * Additionally, break the reads up so that they do not cross
    -+		 * a page boundary.
    ++		 * Additionally, a read from the Shadow RAM may not cross over
    ++		 * a sector boundary. Conveniently, the sector size is also
    ++		 * 4Kb.
     +		 */
    -+		page_offset = offset % ICE_AQ_MAX_BUF_LEN;
    -+		read_size = min_t(u32, ICE_AQ_MAX_BUF_LEN - page_offset,
    ++		sector_offset = offset % ICE_AQ_MAX_BUF_LEN;
    ++		read_size = min_t(u32, ICE_AQ_MAX_BUF_LEN - sector_offset,
     +				  inlen - bytes_read);
     +
    -+		if ((bytes_read + read_size) < inlen)
    -+			last_cmd = false;
    ++		last_cmd = !(bytes_read + read_size < inlen);
     +
     +		status = ice_aq_read_nvm(hw, ICE_AQC_NVM_START_POINT,
     +					 offset, read_size,
    @@ drivers/net/ethernet/intel/ice/ice_nvm.c: ice_read_sr_aq(struct ice_hw *hw, u32
     -	status = ice_read_sr_aq(hw, offset, 1, &data_local, true);
     -	if (!status)
     -		*data = le16_to_cpu(data_local);
    -+	/* Note that ice_read_flat_nvm checks if the read is past the Shadow
    -+	 * RAM size, and ensures we don't read across a page boundary
    ++	/* Note that ice_read_flat_nvm takes into account the 4Kb AdminQ and
    ++	 * Shadow RAM sector restrictions necessary when reading from the NVM.
     +	 */
     +	status = ice_read_flat_nvm(hw, offset * sizeof(u16), &bytes,
     +				   (u8 *)&data_local, true);
 7:  5f4f6ba0e561 =  3:  54ef31b469ee ice: implement full NVM read from ETHTOOL_GEEPROM
 9:  6bc459c7ade7 !  4:  58059efb5936 ice: enable initial devlink support
    @@ Commit message
         ice: enable initial devlink support
     
         Begin implementing support for the devlink interface with the ice
    -    driver. Use devlinkm_alloc to allocate the devlink memory. The PF
    -    private data structure is now allocated as part of the devlink instead
    -    of as a standalone allocation.
    +    driver.
    +
    +    The pf structure is currently memory managed through devres, via
    +    a devm_alloc. To mimic this behavior, after allocating the devlink
    +    pointer, use devm_add_action to add a teardown action for releasing the
    +    devlink memory on exit.
     
         The ice hardware is a multi-function PCIe device. Thus, each physical
         function will get its own devlink instance. This means that each
    @@ Commit message
         configuration. This is done because the ice driver loads a separate
         instance for each function.
     
    -    That means that this implementation does not enable devlink to manage
    +    Due to this, the implementation does not enable devlink to manage
         device-wide resources or configuration, as each physical function will
         be treated independently. This is done for simplicity, as managing
         a devlink instance across multiple driver instances would significantly
    @@ drivers/net/ethernet/intel/ice/ice_devlink.c (new)
     +const struct devlink_ops ice_devlink_ops = {
     +};
     +
    ++static void ice_devlink_free(void *devlink_ptr)
    ++{
    ++	devlink_free((struct devlink *)devlink_ptr);
    ++}
    ++
    ++/**
    ++ * ice_allocate_pf - Allocate devlink and return PF structure pointer
    ++ * @dev: the device to allocate for
    ++ *
    ++ * Allocate a devlink instance for this device and return the private area as
    ++ * the PF structure. The devlink memory is kept track of through devres by
    ++ * adding an action to remove it when unwinding.
    ++ */
    ++struct ice_pf *ice_allocate_pf(struct device *dev)
    ++{
    ++	struct devlink *devlink;
    ++
    ++	devlink = devlink_alloc(&ice_devlink_ops, sizeof(struct ice_pf));
    ++	if (!devlink)
    ++		return NULL;
    ++
    ++	/* Add an action to teardown the devlink when unwinding the driver */
    ++	if (devm_add_action(dev, ice_devlink_free, devlink)) {
    ++		devlink_free(devlink);
    ++		return NULL;
    ++	}
    ++
    ++	return devlink_priv(devlink);
    ++}
    ++
     +/**
     + * ice_devlink_register - Register devlink interface for this PF
     + * @pf: the PF to register the devlink for.
    @@ drivers/net/ethernet/intel/ice/ice_devlink.c (new)
     +}
     +
     +/**
    -+ * ice_devlink_unregister - Unregister devlink resources for this pf.
    ++ * ice_devlink_unregister - Unregister devlink resources for this PF.
     + * @pf: the PF structure to cleanup
     + *
     + * Releases resources used by devlink and cleans up associated memory.
    @@ drivers/net/ethernet/intel/ice/ice_devlink.c (new)
     + * @pf: the PF to create a port for
     + *
     + * Create and register a devlink_port for this PF. Note that although each
    -+ * physical function connected to a separate devlink instance, the port will
    -+ * still be numbered according to the physical function id.
    ++ * physical function is connected to a separate devlink instance, the port
    ++ * will still be numbered according to the physical function id.
     + *
     + * @returns zero on success or an error code on failure.
     + */
    @@ drivers/net/ethernet/intel/ice/ice_devlink.c (new)
     +	int err;
     +
     +	if (!vsi) {
    -+		dev_warn(dev, "%s: unable to find main VSI\n", __func__);
    ++		dev_err(dev, "%s: unable to find main VSI\n", __func__);
     +		return -EIO;
     +	}
     +
    @@ drivers/net/ethernet/intel/ice/ice_devlink.c (new)
     +		dev_err(dev, "devlink_port_register failed: %d\n", err);
     +		return err;
     +	}
    -+	devlink_port_type_eth_set(&pf->devlink_port, vsi->netdev);
    ++	if (vsi->netdev)
    ++		devlink_port_type_eth_set(&pf->devlink_port, vsi->netdev);
     +
     +	return 0;
     +}
    @@ drivers/net/ethernet/intel/ice/ice_devlink.h (new)
     +#ifndef _ICE_DEVLINK_H_
     +#define _ICE_DEVLINK_H_
     +
    -+extern const struct devlink_ops ice_devlink_ops;
    ++struct ice_pf *ice_allocate_pf(struct device *dev);
     +
     +int ice_devlink_register(struct ice_pf *pf);
     +void ice_devlink_unregister(struct ice_pf *pf);
    @@ drivers/net/ethernet/intel/ice/ice_main.c
      
      #define DRV_VERSION_MAJOR 0
      #define DRV_VERSION_MINOR 8
    -@@ drivers/net/ethernet/intel/ice/ice_main.c: static int ice_setup_pf_sw(struct ice_pf *pf)
    - 		status = -ENODEV;
    - 		goto unroll_vsi_setup;
    - 	}
    -+
    -+	status = ice_devlink_create_port(pf);
    -+	if (status)
    -+		goto unroll_vsi_setup;
    -+
    - 	/* netdev has to be configured before setting frame size */
    - 	ice_vsi_cfg_frame_size(vsi);
    - 
    -@@ drivers/net/ethernet/intel/ice/ice_main.c: static int ice_setup_pf_sw(struct ice_pf *pf)
    - 		}
    - 	}
    - 
    -+	ice_devlink_destroy_port(pf);
    -+
    - unroll_vsi_setup:
    - 	if (vsi) {
    - 		ice_vsi_free_q_vectors(vsi);
    -@@ drivers/net/ethernet/intel/ice/ice_main.c: static int
    - ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
    - {
    - 	struct device *dev = &pdev->dev;
    -+	struct devlink *devlink;
    - 	struct ice_pf *pf;
    - 	struct ice_hw *hw;
    - 	int err;
     @@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
      		return err;
      	}
      
     -	pf = devm_kzalloc(dev, sizeof(*pf), GFP_KERNEL);
    --	if (!pf)
    -+	devlink = devlinkm_alloc(dev, &ice_devlink_ops, sizeof(*pf));
    -+	if (!devlink) {
    -+		dev_err(dev, "devlink allocation failed\n");
    ++	pf = ice_allocate_pf(dev);
    + 	if (!pf)
      		return -ENOMEM;
    -+	}
      
    - 	/* set up for high or low DMA */
    - 	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
    -@@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
    - 	pci_enable_pcie_error_reporting(pdev);
    - 	pci_set_master(pdev);
    - 
    -+	pf = devlink_priv(devlink);
    - 	pf->pdev = pdev;
    - 	pci_set_drvdata(pdev, pf);
    - 	set_bit(__ICE_DOWN, pf->state);
     @@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
      
      	pf->msg_enable = netif_msg_init(debug, ICE_DFLT_NETIF_M);
    @@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const
      #ifndef CONFIG_DYNAMIC_DEBUG
      	if (debug < -1)
      		hw->debug_mask = debug;
    +@@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
    + 		goto err_alloc_sw_unroll;
    + 	}
    + 
    ++	err = ice_devlink_create_port(pf);
    ++	if (err)
    ++		goto err_alloc_sw_unroll;
    ++
    ++
    + 	clear_bit(__ICE_SERVICE_DIS, pf->state);
    + 
    + 	/* tell the firmware we are up */
    +@@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
    + 	return 0;
    + 
    + err_alloc_sw_unroll:
    ++	ice_devlink_destroy_port(pf);
    + 	set_bit(__ICE_SERVICE_DIS, pf->state);
    + 	set_bit(__ICE_DOWN, pf->state);
    + 	devm_kfree(dev, pf->first_sw);
     @@ drivers/net/ethernet/intel/ice/ice_main.c: ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
      	ice_deinit_pf(pf);
      	ice_deinit_hw(hw);
 -:  ------------ >  5:  0b22901ddc9a ice: rename variables used for Option ROM version
 -:  ------------ >  6:  94e187ff9f4d ice: add basic handler for devlink .info_get
11:  e386119abbfb !  7:  59408e666b26 ice: add board identifier info to devlink .info_get
    @@ Commit message
     
         Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
     
    + ## Documentation/networking/devlink/ice.rst ##
    +@@ Documentation/networking/devlink/ice.rst: The ``ice`` driver reports the following versions
    +       - Type
    +       - Example
    +       - Description
    ++    * - ``board.id``
    ++      - fixed
    ++      - K65390-000
    ++      - The Product Board Assembly (PBA) identifier of the board.
    +     * - ``fw.mgmt``
    +       - running
    +       - 1.16.10
    +@@ Documentation/networking/devlink/ice.rst: The ``ice`` driver reports the following versions
    +         are supported.
    +     * - ``fw.mgmt.bundle``
    +       - running
    +-      - ecabd066
    +-      - Unique identifier of the management firmware.
    ++      - 0xecabd066
    ++      - Unique identifier of the management firmware build.
    +     * - ``fw.undi.orom``
    +       - running
    +       - 1.2186.0
    +       - Version of the Option ROM containing the UEFI driver. The version is
    +         reported in ``major.minor.patch`` format. The major version is
    +-        incremented whenever a major breaking change occurs, or when
    +-        the minor version would overflow. The minor version is incremented
    +-        for non-breaking changes, and is reset to 1 when the major version
    +-        is incremented. The patch version is normally 0 but is incremented
    +-        when a fix is delivered as a patch against an older base Option ROM.
    ++        incremented whenever a major breaking change occurs, or when the
    ++        minor version would overflow. The minor version is incremented for
    ++        non-breaking changes and reset to 1 when the major version is
    ++        incremented. The patch version is normally 0 but is incremented when
    ++        a fix is delivered as a patch against an older base Option ROM.
    +     * - ``nvm.psid``
    +       - running
    +       - 0.50
    +-      - Version of the format for the NVM parameter set
    ++      - Version describing the format of the NVM parameter set.
    +     * - ``nvm.bundle``
    +       - running
    +       - 0x80001709
    +-      - Unique identifier for the entire NVM image contents, also known as
    +-        the EETRACK id.
    ++      - Unique identifier of the NVM image contents, also known as the
    ++        EETRACK id.
    +
      ## drivers/net/ethernet/intel/ice/ice_common.c ##
     @@ drivers/net/ethernet/intel/ice/ice_common.c: enum ice_status ice_reset(struct ice_hw *hw, enum ice_reset_req req)
      	return ice_check_reset(hw);
    @@ drivers/net/ethernet/intel/ice/ice_common.h: enum ice_status ice_nvm_validate_ch
     
      ## drivers/net/ethernet/intel/ice/ice_devlink.c ##
     @@ drivers/net/ethernet/intel/ice/ice_devlink.c: static int ice_devlink_info_get(struct devlink *devlink,
    - 	u8 oem_ver, oem_patch, nvm_ver_hi, nvm_ver_lo;
    + 	u8 orom_maj, orom_patch, nvm_ver_hi, nvm_ver_lo;
      	struct ice_pf *pf = devlink_priv(devlink);
      	struct ice_hw *hw = &pf->hw;
     +	enum ice_status status;
    - 	u16 oem_build;
    - 	char buf[32]; /* TODO: size this properly */
    + 	u16 orom_min;
    +-	char buf[32]; /* TODO: size this properly */
    ++	char buf[32];
      	int err;
    + 
    + 	ice_get_nvm_version(hw, &orom_maj, &orom_min, &orom_patch, &nvm_ver_hi,
     @@ drivers/net/ethernet/intel/ice/ice_devlink.c: static int ice_devlink_info_get(struct devlink *devlink,
      		return err;
      	}
    @@ drivers/net/ethernet/intel/ice/ice_devlink.c: static int ice_devlink_info_get(st
     +		return -EIO;
     +	}
     +
    -+	/* board.id (DEVLINK_INFO_VERSION_GENERIC_BOARD_ID) */
    -+	err = devlink_info_version_fixed_put(req, "board.id", buf);
    ++	err = devlink_info_version_fixed_put(req,
    ++					     DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,
    ++					     buf);
     +	if (err) {
     +		NL_SET_ERR_MSG_MOD(extack, "Unable to set board identifier");
     +		return err;
     +	}
     +
    - 	/* fw (match exact output of ethtool -i firmware-version) */
    + 	snprintf(buf, sizeof(buf), "%u.%u.%u", hw->fw_maj_ver, hw->fw_min_ver,
    + 		 hw->fw_patch);
      	err = devlink_info_version_running_put(req,
    - 					       DEVLINK_INFO_VERSION_GENERIC_FW,
    +@@ drivers/net/ethernet/intel/ice/ice_devlink.c: static int ice_devlink_info_get(struct devlink *devlink,
    + 		return err;
    + 	}
    + 
    +-	snprintf(buf, sizeof(buf), "%u.%u", hw->api_maj_ver,
    +-		 hw->api_min_ver);
    ++	snprintf(buf, sizeof(buf), "%u.%u", hw->api_maj_ver, hw->api_min_ver);
    + 	err = devlink_info_version_running_put(req, "fw.mgmt.api", buf);
    + 	if (err) {
    + 		NL_SET_ERR_MSG_MOD(extack, "Unable to set mgmt fw API data");
     
      ## drivers/net/ethernet/intel/ice/ice_nvm.c ##
     @@ drivers/net/ethernet/intel/ice/ice_nvm.c: enum ice_status ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data)
    @@ drivers/net/ethernet/intel/ice/ice_nvm.c: enum ice_status ice_read_sr_word(struc
     +	 */
     +	pba_size--;
     +	if (pba_num_size < (((u32)pba_size * 2) + 1)) {
    -+		ice_debug(hw, ICE_DBG_INIT,
    -+			  "Buffer too small for PBA data.\n");
    ++		ice_debug(hw, ICE_DBG_INIT, "Buffer too small for PBA data.\n");
     +		return ICE_ERR_PARAM;
     +	}
     +
     +	for (i = 0; i < pba_size; i++) {
     +		status = ice_read_sr_word(hw, (pba_tlv + 2 + 1) + i, &pba_word);
     +		if (status) {
    -+			ice_debug(hw, ICE_DBG_INIT,
    -+				  "Failed to read PBA Block word %d.\n", i);
    ++			ice_debug(hw, ICE_DBG_INIT, "Failed to read PBA Block word %d.\n", i);
     +			return status;
     +		}
     +
    @@ drivers/net/ethernet/intel/ice/ice_type.h
     @@ drivers/net/ethernet/intel/ice/ice_type.h: struct ice_hw_port_stats {
      /* Checksum and Shadow RAM pointers */
      #define ICE_SR_BOOT_CFG_PTR		0x132
    - #define ICE_NVM_OEM_VER_OFF		0x02
    + #define ICE_NVM_OROM_VER_OFF		0x02
     +#define ICE_SR_PBA_BLOCK_PTR		0x16
      #define ICE_SR_NVM_DEV_STARTER_VER	0x18
      #define ICE_SR_NVM_EETRACK_LO		0x2D
 1:  7d571fe7498b !  8:  1b745d45484b devlink: prepare to support region operations
    @@ Commit message
         the future such as requesting snapshots, or accessing the region
         directly without a snapshot.
     
    +    In order to re-use the constant strings in the mlx4 driver their
    +    declaration must be changed to 'const char * const' to ensure the
    +    compiler realizes that both the data and the pointer cannot change.
    +
         Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
    +    Reviewed-by: Jakub Kicinski <kuba@kernel.org>
     
      ## drivers/net/ethernet/mellanox/mlx4/crdump.c ##
     @@
    @@ drivers/net/ethernet/mellanox/mlx4/crdump.c
      
     -static const char *region_cr_space_str = "cr-space";
     -static const char *region_fw_health_str = "fw-health";
    ++static const char * const region_cr_space_str = "cr-space";
    ++static const char * const region_fw_health_str = "fw-health";
    ++
     +static const struct devlink_region_ops region_cr_space_ops = {
    -+	.name = "cr-space",
    ++	.name = region_cr_space_str,
     +};
     +
     +static const struct devlink_region_ops region_fw_health_ops = {
    -+	.name = "fw-health",
    ++	.name = region_fw_health_str,
     +};
      
      /* Set to true in case cr enable bit was set to true before crdump */
      static bool crdump_enbale_bit_set;
    -@@ drivers/net/ethernet/mellanox/mlx4/crdump.c: static void mlx4_crdump_collect_crspace(struct mlx4_dev *dev,
    - 		if (err) {
    - 			kvfree(crspace_data);
    - 			mlx4_warn(dev, "crdump: devlink create %s snapshot id %d err %d\n",
    --				  region_cr_space_str, id, err);
    -+				  region_cr_space_ops.name, id, err);
    - 		} else {
    - 			mlx4_info(dev, "crdump: added snapshot %d to devlink region %s\n",
    --				  id, region_cr_space_str);
    -+				  id, region_cr_space_ops.name);
    - 		}
    - 	} else {
    - 		mlx4_err(dev, "crdump: Failed to allocate crspace buffer\n");
    -@@ drivers/net/ethernet/mellanox/mlx4/crdump.c: static void mlx4_crdump_collect_fw_health(struct mlx4_dev *dev,
    - 		if (err) {
    - 			kvfree(health_data);
    - 			mlx4_warn(dev, "crdump: devlink create %s snapshot id %d err %d\n",
    --				  region_fw_health_str, id, err);
    -+				  region_fw_health_ops.name, id, err);
    - 		} else {
    - 			mlx4_info(dev, "crdump: added snapshot %d to devlink region %s\n",
    --				  id, region_fw_health_str);
    -+				  id, region_fw_health_ops.name);
    - 		}
    - 	} else {
    - 		mlx4_err(dev, "crdump: Failed to allocate health buffer\n");
     @@ drivers/net/ethernet/mellanox/mlx4/crdump.c: int mlx4_crdump_init(struct mlx4_dev *dev)
      	/* Create cr-space region */
      	crdump->region_crspace =
    @@ drivers/net/ethernet/mellanox/mlx4/crdump.c: int mlx4_crdump_init(struct mlx4_de
      				      MAX_NUM_OF_DUMPS_TO_STORE,
      				      pci_resource_len(pdev, 0));
      	if (IS_ERR(crdump->region_crspace))
    - 		mlx4_warn(dev, "crdump: create devlink region %s err %ld\n",
    --			  region_cr_space_str,
    -+			  region_cr_space_ops.name,
    - 			  PTR_ERR(crdump->region_crspace));
    - 
    +@@ drivers/net/ethernet/mellanox/mlx4/crdump.c: int mlx4_crdump_init(struct mlx4_dev *dev)
      	/* Create fw-health region */
      	crdump->region_fw_health =
      		devlink_region_create(devlink,
    @@ drivers/net/ethernet/mellanox/mlx4/crdump.c: int mlx4_crdump_init(struct mlx4_de
      				      MAX_NUM_OF_DUMPS_TO_STORE,
      				      HEALTH_BUFFER_SIZE);
      	if (IS_ERR(crdump->region_fw_health))
    - 		mlx4_warn(dev, "crdump: create devlink region %s err %ld\n",
    --			  region_fw_health_str,
    -+			  region_fw_health_ops.name,
    - 			  PTR_ERR(crdump->region_fw_health));
    - 
    - 	return 0;
     
      ## drivers/net/netdevsim/dev.c ##
     @@ drivers/net/netdevsim/dev.c: static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink)
    @@ include/net/devlink.h: void devlink_port_param_value_changed(struct devlink_port
     +struct devlink_region *
     +devlink_region_create(struct devlink *devlink,
     +		      const struct devlink_region_ops *ops,
    -+		      u32 region_max_snapshots,
    -+		      u64 region_size);
    ++		      u32 region_max_snapshots, u64 region_size);
      void devlink_region_destroy(struct devlink_region *region);
      u32 devlink_region_snapshot_id_get(struct devlink *devlink);
      int devlink_region_snapshot_create(struct devlink_region *region,
    @@ net/core/devlink.c: EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
     +struct devlink_region *
     +devlink_region_create(struct devlink *devlink,
     +		      const struct devlink_region_ops *ops,
    -+		      u32 region_max_snapshots,
    -+		      u64 region_size)
    ++		      u32 region_max_snapshots, u64 region_size)
      {
      	struct devlink_region *region;
      	int err = 0;
 -:  ------------ >  9:  9032cc32d7b0 devlink: convert snapshot destructor callback to region op
 -:  ------------ > 10:  0733d5acd4eb devlink: trivial: fix tab in function documentation
 2:  5a532f335927 ! 11:  db000f11c121 devlink: add functions to take snapshot while locked
    @@ Commit message
         devlink_region_snapshot_id_get and devlink_region_snapshot_create
         functions while already holding the devlink instance lock.
     
    -    Extract the logic of these two functions into static functions with the
    -    _locked postfix. Modify the original functions to be implemented in
    -    terms of the new locked functions.
    +    Extract the logic of these two functions into static functions prefixed
    +    by `__` to indicate they are internal helper functions. Modify the
    +    original functions to be implemented in terms of the new locked
    +    functions.
     
         Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
     
    @@ net/core/devlink.c: static void devlink_nl_region_notify(struct devlink_region *
      }
      
     +/**
    -+ *	devlink_region_snapshot_id_get_locked - get snapshot ID
    ++ *	__devlink_region_snapshot_id_get - get snapshot ID
    ++ *	@devlink: devlink instance
     + *
     + *	Returns a new snapshot id. Must be called while holding the
     + *	devlink instance lock.
     + */
    -+static u32 devlink_region_snapshot_id_get_locked(struct devlink *devlink)
    ++static u32 __devlink_region_snapshot_id_get(struct devlink *devlink)
     +{
    ++	lockdep_assert_held(&devlink->lock);
     +	return ++devlink->snapshot_id;
     +}
     +
     +/**
    -+ *	devlink_region_snapshot_create_locked - create a new snapshot
    ++ *	__devlink_region_snapshot_create - create a new snapshot
     + *	This will add a new snapshot of a region. The snapshot
     + *	will be stored on the region struct and can be accessed
    -+ *	from devlink. This is useful for future	analyses of snapshots.
    ++ *	from devlink. This is useful for future analyses of snapshots.
     + *	Multiple snapshots can be created on a region.
     + *	The @snapshot_id should be obtained using the getter function.
     + *
    @@ net/core/devlink.c: static void devlink_nl_region_notify(struct devlink_region *
     + *	@region: devlink region of the snapshot
     + *	@data: snapshot data
     + *	@snapshot_id: snapshot id to be created
    -+ *	@destructor: pointer to destructor function to free data
     + */
     +static int
    -+devlink_region_snapshot_create_locked(struct devlink_region *region,
    -+				      u8 *data, u32 snapshot_id,
    -+				      devlink_snapshot_data_dest_t *destructor)
    ++__devlink_region_snapshot_create(struct devlink_region *region,
    ++				 u8 *data, u32 snapshot_id)
     +{
    ++	struct devlink *devlink = region->devlink;
     +	struct devlink_snapshot *snapshot;
     +
    ++	lockdep_assert_held(&devlink->lock);
    ++
     +	/* check if region can hold one more snapshot */
     +	if (region->cur_snapshots == region->max_snapshots)
     +		return -ENOMEM;
    @@ net/core/devlink.c: static void devlink_nl_region_notify(struct devlink_region *
     +	snapshot->id = snapshot_id;
     +	snapshot->region = region;
     +	snapshot->data = data;
    -+	snapshot->data_destructor = destructor;
     +
     +	list_add_tail(&snapshot->list, &region->snapshot_list);
     +
    @@ net/core/devlink.c: u32 devlink_region_snapshot_id_get(struct devlink *devlink)
      
      	mutex_lock(&devlink->lock);
     -	id = ++devlink->snapshot_id;
    -+	id = devlink_region_snapshot_id_get_locked(devlink);
    ++	id = __devlink_region_snapshot_id_get(devlink);
      	mutex_unlock(&devlink->lock);
      
      	return id;
    -@@ net/core/devlink.c: EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
    -  *	devlink_region_snapshot_create - create a new snapshot
    -  *	This will add a new snapshot of a region. The snapshot
    -  *	will be stored on the region struct and can be accessed
    -- *	from devlink. This is useful for future	analyses of snapshots.
    -+ *	from devlink. This is useful for future analyses of snapshots.
    -  *	Multiple snapshots can be created on a region.
    -  *	The @snapshot_id should be obtained using the getter function.
    -  *
     @@ net/core/devlink.c: int devlink_region_snapshot_create(struct devlink_region *region,
    - 				   devlink_snapshot_data_dest_t *data_destructor)
    + 				   u8 *data, u32 snapshot_id)
      {
      	struct devlink *devlink = region->devlink;
     -	struct devlink_snapshot *snapshot;
    @@ net/core/devlink.c: int devlink_region_snapshot_create(struct devlink_region *re
     -	snapshot->id = snapshot_id;
     -	snapshot->region = region;
     -	snapshot->data = data;
    --	snapshot->data_destructor = data_destructor;
     -
     -	list_add_tail(&snapshot->list, &region->snapshot_list);
     -
     -	region->cur_snapshots++;
     -
     -	devlink_nl_region_notify(region, snapshot, DEVLINK_CMD_REGION_NEW);
    -+	err = devlink_region_snapshot_create_locked(region, data, snapshot_id,
    -+						    data_destructor);
    ++	err = __devlink_region_snapshot_create(region, data, snapshot_id);
      	mutex_unlock(&devlink->lock);
     -	return 0;
      
 3:  806a97ae3de9 <  -:  ------------ devlink: add operation to take an immediate snapshot
 4:  b4276446fdcf <  -:  ------------ netdevsim: support taking immediate snapshot via devlink
 8:  f3141a755fb5 <  -:  ------------ devlink: add devres managed devlinkm_alloc and devlinkm_free
10:  d1284fd5b0ee <  -:  ------------ ice: add basic handler for devlink .info_get
12:  30a621018ac2 <  -:  ------------ ice: add a devlink region to dump shadow RAM contents
13:  cb1b4d27d9af <  -:  ------------ devlink: support directly reading from region memory
14:  feae26ff3541 <  -:  ------------ ice: support direct read of the shadow ram region
15:  1e7c2cd5fb66 <  -:  ------------ ice: add ice.rst devlink documentation file
 -:  ------------ > 12:  192d7644d59f devlink: convert snapshot id getter to return an error
 -:  ------------ > 13:  37b91ca05e63 devlink: track snapshot ids using an IDR and refcounts
 -:  ------------ > 14:  cf6472e590b0 devlink: implement DEVLINK_CMD_REGION_NEW
 -:  ------------ > 15:  3371776f00a3 netdevsim: support taking immediate snapshot via devlink
 -:  ------------ > 16:  3017e55058e1 devlink: simplify arguments for read_snapshot_fill
 -:  ------------ > 17:  d1ef960f156c devlink: use min_t to calculate data_size
 -:  ------------ > 18:  06c791e0df4d devlink: report extended error message in region_read_dumpit
 -:  ------------ > 19:  5aa4cee09a1f devlink: remove unnecessary parameter from chunk_fill function
 -:  ------------ > 20:  2eb06cab901b devlink: refactor region_read_snapshot_fill to use a callback function
 -:  ------------ > 21:  854875dd7872 devlink: support directly reading from region memory
 -:  ------------ > 22:  0ebf8548ddb2 ice: add a devlink region to dump shadow RAM contents