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

Jacob Keller
2020-03-02T19:27:09Z

[-- 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