diff mbox series

[net-next,v5,06/10] bnxt: Add devlink support for config get/set

Message ID 1509374776-45869-7-git-send-email-steven.lin1@broadcom.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series Adding permanent config get/set to devlink | expand

Commit Message

Steve Lin Oct. 30, 2017, 2:46 p.m. UTC 
Implements get and set of configuration parameters using new devlink
config get/set API. Parameters themselves defined in later patches.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 2 files changed, 263 insertions(+), 12 deletions(-)

Comments

Jiri Pirko Oct. 30, 2017, 5:35 p.m. UTC | #1 
Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:
>Implements get and set of configuration parameters using new devlink
>config get/set API. Parameters themselves defined in later patches.
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
> 2 files changed, 263 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 402fa32..deb24e0 100644

[...]

>+static int bnxt_dl_perm_config_set(struct devlink *devlink,
>+				   enum devlink_perm_config_param param,
>+				   enum devlink_perm_config_type type,
>+				   union devlink_perm_config_value *value,
>+				   bool *restart_reqd)
>+{
>+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
>+	struct bnxt_drv_cfgparam *entry = NULL;
>+	u32 value_int;
>+	u32 bytesize;
>+	int idx = 0;
>+	int ret = 0;
>+	u16 val16;
>+	u8 val8;
>+	int i;
>+
>+	*restart_reqd = 0;
>+
>+	/* Find parameter in table */
>+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
>+		if (param == bnxt_drv_cfgparam_list[i].param) {
>+			entry = &bnxt_drv_cfgparam_list[i];
>+			break;
>+		}
>+	}
>+
>+	/* Not found */
>+	if (!entry)
>+		return -EINVAL;
>+
>+	/* Check to see if this func type can access variable */
>+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
>+		return -EOPNOTSUPP;
>+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
>+		return -EOPNOTSUPP;
>+
>+	/* If parameter is per port or function, compute index */
>+	if (entry->appl == BNXT_DRV_APPL_PORT) {
>+		idx = bp->pf.port_id;
>+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
>+		if (BNXT_PF(bp))
>+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
> 	}
> 
>+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
>+
>+	/* Type expected by device may or may not be the same as type passed
>+	 * in from devlink API.  So first convert to an interval u32 value,
>+	 * then to type needed by device.
>+	 */
>+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
>+		value_int = value->value8;
>+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
>+		value_int = value->value16;
>+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
>+		value_int = value->value32;
>+	} else {
>+		/* Unsupported type */
>+		return -EINVAL;
>+	}
>+
>+	if (bytesize == 1) {
>+		val8 = value_int;

>+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
>+				     entry->bitlength);
>+	} else if (bytesize == 2) {
>+		val16 = value_int;
>+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
>+				     entry->bitlength);
>+	} else {
>+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
>+				     entry->bitlength);
>+	}
>+
>+	/* Restart required for all nvm parameter writes */
>+	*restart_reqd = 1;
>+
>+	return ret;
>+}

I don't like this swissknife approach for setters and getters. It's
messy, and hard to read. There should be clean get/set pair function
per parameter defined in array.
Something like:

static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
{
	...
}

static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
					bool *restart_reqd)
{
	...
}

static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
{
	...
}

static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
					bool *restart_reqd)
{
	...
}

static const struct devlink_config_param bnxt_params[] = {
	DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
				  bnxt_param_sriov_enabled_get,
				  bnxt_param_sriov_enabled_set),
	DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
				 bnxt_param_num_vf_per_pf_get,
				 bnxt_param_num_vf_per_pf_set),
};

and then in init:

err = devlink_config_param_register(devlink, bnxt_params,
				    ARRAY_SIZE(bnxt_params));

The register function will check types against the internal devlink
param->type table.

Also, the restart_reqd could be signalized by some pre-defined positive
setter return value.
Steve Lin Oct. 30, 2017, 8:20 p.m. UTC | #2 
On Mon, Oct 30, 2017 at 1:35 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:
>>Implements get and set of configuration parameters using new devlink
>>config get/set API. Parameters themselves defined in later patches.
>>
>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>---
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
>> 2 files changed, 263 insertions(+), 12 deletions(-)
>>
>>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>>index 402fa32..deb24e0 100644
>
> [...]
>
>>+static int bnxt_dl_perm_config_set(struct devlink *devlink,
>>+                                 enum devlink_perm_config_param param,
>>+                                 enum devlink_perm_config_type type,
>>+                                 union devlink_perm_config_value *value,
>>+                                 bool *restart_reqd)
>>+{
>>+      struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
>>+      struct bnxt_drv_cfgparam *entry = NULL;
>>+      u32 value_int;
>>+      u32 bytesize;
>>+      int idx = 0;
>>+      int ret = 0;
>>+      u16 val16;
>>+      u8 val8;
>>+      int i;
>>+
>>+      *restart_reqd = 0;
>>+
>>+      /* Find parameter in table */
>>+      for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
>>+              if (param == bnxt_drv_cfgparam_list[i].param) {
>>+                      entry = &bnxt_drv_cfgparam_list[i];
>>+                      break;
>>+              }
>>+      }
>>+
>>+      /* Not found */
>>+      if (!entry)
>>+              return -EINVAL;
>>+
>>+      /* Check to see if this func type can access variable */
>>+      if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
>>+              return -EOPNOTSUPP;
>>+      if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
>>+              return -EOPNOTSUPP;
>>+
>>+      /* If parameter is per port or function, compute index */
>>+      if (entry->appl == BNXT_DRV_APPL_PORT) {
>>+              idx = bp->pf.port_id;
>>+      } else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
>>+              if (BNXT_PF(bp))
>>+                      idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
>>       }
>>
>>+      bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
>>+
>>+      /* Type expected by device may or may not be the same as type passed
>>+       * in from devlink API.  So first convert to an interval u32 value,
>>+       * then to type needed by device.
>>+       */
>>+      if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
>>+              value_int = value->value8;
>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
>>+              value_int = value->value16;
>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
>>+              value_int = value->value32;
>>+      } else {
>>+              /* Unsupported type */
>>+              return -EINVAL;
>>+      }
>>+
>>+      if (bytesize == 1) {
>>+              val8 = value_int;
>
>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
>>+                                   entry->bitlength);
>>+      } else if (bytesize == 2) {
>>+              val16 = value_int;
>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
>>+                                   entry->bitlength);
>>+      } else {
>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
>>+                                   entry->bitlength);
>>+      }
>>+
>>+      /* Restart required for all nvm parameter writes */
>>+      *restart_reqd = 1;
>>+
>>+      return ret;
>>+}
>
> I don't like this swissknife approach for setters and getters. It's
> messy, and hard to read. There should be clean get/set pair function
> per parameter defined in array.

The advantage of the swiss kinfe approach is that you don't have a
large number of almost-identical functions (i.e. any "set" that
operates on a u32 will be doing essentially the exact same thing) in
the driver, which could lead to code bloat and also make it harder to
catch errors (since any bug that may creep in during the copy/paste to
make so many functions will only be caught when that specific
parameter is set, rather than when any parameter is set).

In general, I very much appreciate the thorough review.  I do feel,
though, like some of this basic architectural stuff could have been
addressed more efficiently in v1 or v2, rather than v5.  I admit that
when the comments to v4 included suggestions like removing extra
spaces in the commit message, I was thinking that we were close to
reaching consensus. :)

I just want to make sure that you feel that the overall goal of this
effort is worthwhile - I will make the changes you've suggested here
and in the other replies to v5, but I am hopeful that v6 does not
result in another set of big architectural changes.

> Something like:
>
> static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
> {
>         ...
> }
>
> static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
>                                         bool *restart_reqd)
> {
>         ...
> }
>
> static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
> {
>         ...
> }
>
> static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
>                                         bool *restart_reqd)
> {
>         ...
> }
>
> static const struct devlink_config_param bnxt_params[] = {
>         DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>                                   bnxt_param_sriov_enabled_get,
>                                   bnxt_param_sriov_enabled_set),
>         DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>                                  bnxt_param_num_vf_per_pf_get,
>                                  bnxt_param_num_vf_per_pf_set),
> };
>
> and then in init:
>
> err = devlink_config_param_register(devlink, bnxt_params,
>                                     ARRAY_SIZE(bnxt_params));
>
> The register function will check types against the internal devlink
> param->type table.
>
> Also, the restart_reqd could be signalized by some pre-defined positive
> setter return value.

Understood - like I said, I thought it preferable to use a global
getter/setter rather than one for each individual parameter, but I
will make the requested change, in hopes of reaching a conclusion
soon.
Jiri Pirko Oct. 31, 2017, 7:19 a.m. UTC | #3 
Mon, Oct 30, 2017 at 09:20:17PM CET, steven.lin1@broadcom.com wrote:
>On Mon, Oct 30, 2017 at 1:35 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:
>>>Implements get and set of configuration parameters using new devlink
>>>config get/set API. Parameters themselves defined in later patches.
>>>
>>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>>---
>>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
>>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
>>> 2 files changed, 263 insertions(+), 12 deletions(-)
>>>
>>>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>>>index 402fa32..deb24e0 100644
>>
>> [...]
>>
>>>+static int bnxt_dl_perm_config_set(struct devlink *devlink,
>>>+                                 enum devlink_perm_config_param param,
>>>+                                 enum devlink_perm_config_type type,
>>>+                                 union devlink_perm_config_value *value,
>>>+                                 bool *restart_reqd)
>>>+{
>>>+      struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
>>>+      struct bnxt_drv_cfgparam *entry = NULL;
>>>+      u32 value_int;
>>>+      u32 bytesize;
>>>+      int idx = 0;
>>>+      int ret = 0;
>>>+      u16 val16;
>>>+      u8 val8;
>>>+      int i;
>>>+
>>>+      *restart_reqd = 0;
>>>+
>>>+      /* Find parameter in table */
>>>+      for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
>>>+              if (param == bnxt_drv_cfgparam_list[i].param) {
>>>+                      entry = &bnxt_drv_cfgparam_list[i];
>>>+                      break;
>>>+              }
>>>+      }
>>>+
>>>+      /* Not found */
>>>+      if (!entry)
>>>+              return -EINVAL;
>>>+
>>>+      /* Check to see if this func type can access variable */
>>>+      if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
>>>+              return -EOPNOTSUPP;
>>>+      if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
>>>+              return -EOPNOTSUPP;
>>>+
>>>+      /* If parameter is per port or function, compute index */
>>>+      if (entry->appl == BNXT_DRV_APPL_PORT) {
>>>+              idx = bp->pf.port_id;
>>>+      } else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
>>>+              if (BNXT_PF(bp))
>>>+                      idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
>>>       }
>>>
>>>+      bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
>>>+
>>>+      /* Type expected by device may or may not be the same as type passed
>>>+       * in from devlink API.  So first convert to an interval u32 value,
>>>+       * then to type needed by device.
>>>+       */
>>>+      if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
>>>+              value_int = value->value8;
>>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
>>>+              value_int = value->value16;
>>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
>>>+              value_int = value->value32;
>>>+      } else {
>>>+              /* Unsupported type */
>>>+              return -EINVAL;
>>>+      }
>>>+
>>>+      if (bytesize == 1) {
>>>+              val8 = value_int;
>>
>>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
>>>+                                   entry->bitlength);
>>>+      } else if (bytesize == 2) {
>>>+              val16 = value_int;
>>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
>>>+                                   entry->bitlength);
>>>+      } else {
>>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
>>>+                                   entry->bitlength);
>>>+      }
>>>+
>>>+      /* Restart required for all nvm parameter writes */
>>>+      *restart_reqd = 1;
>>>+
>>>+      return ret;
>>>+}
>>
>> I don't like this swissknife approach for setters and getters. It's
>> messy, and hard to read. There should be clean get/set pair function
>> per parameter defined in array.
>
>The advantage of the swiss kinfe approach is that you don't have a
>large number of almost-identical functions (i.e. any "set" that
>operates on a u32 will be doing essentially the exact same thing) in
>the driver, which could lead to code bloat and also make it harder to
>catch errors (since any bug that may creep in during the copy/paste to
>make so many functions will only be caught when that specific
>parameter is set, rather than when any parameter is set).

You can always do some common functions to do the work. But if gives the
driver freedom, unlike the swissknife approach.


>
>In general, I very much appreciate the thorough review.  I do feel,
>though, like some of this basic architectural stuff could have been
>addressed more efficiently in v1 or v2, rather than v5.  I admit that
>when the comments to v4 included suggestions like removing extra
>spaces in the commit message, I was thinking that we were close to
>reaching consensus. :)

Yeah, I noticed this just now.

>
>I just want to make sure that you feel that the overall goal of this
>effort is worthwhile - I will make the changes you've suggested here
>and in the other replies to v5, but I am hopeful that v6 does not
>result in another set of big architectural changes.

Hopefully.


>
>> Something like:
>>
>> static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
>> {
>>         ...
>> }
>>
>> static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
>>                                         bool *restart_reqd)
>> {
>>         ...
>> }
>>
>> static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
>> {
>>         ...
>> }
>>
>> static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
>>                                         bool *restart_reqd)
>> {
>>         ...
>> }
>>
>> static const struct devlink_config_param bnxt_params[] = {
>>         DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>                                   bnxt_param_sriov_enabled_get,
>>                                   bnxt_param_sriov_enabled_set),
>>         DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>>                                  bnxt_param_num_vf_per_pf_get,
>>                                  bnxt_param_num_vf_per_pf_set),
>> };
>>
>> and then in init:
>>
>> err = devlink_config_param_register(devlink, bnxt_params,
>>                                     ARRAY_SIZE(bnxt_params));
>>
>> The register function will check types against the internal devlink
>> param->type table.
>>
>> Also, the restart_reqd could be signalized by some pre-defined positive
>> setter return value.
>
>Understood - like I said, I thought it preferable to use a global
>getter/setter rather than one for each individual parameter, but I
>will make the requested change, in hopes of reaching a conclusion
>soon.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 402fa32..deb24e0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,26 +14,260 @@ 
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
-static const struct devlink_ops bnxt_dl_ops = {
-#ifdef CONFIG_BNXT_SRIOV
-	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
-	.eswitch_mode_get = bnxt_dl_eswitch_mode_get,
-#endif /* CONFIG_BNXT_SRIOV */
+struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 };
 
-int bnxt_dl_register(struct bnxt *bp)
+#define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
+
+static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
+			 void *buf, int size)
 {
-	struct devlink *dl;
+	struct hwrm_nvm_get_variable_input req = {0};
+	dma_addr_t dest_data_dma_addr;
+	void *dest_data_addr = NULL;
+	int bytesize;
 	int rc;
 
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
+	bytesize = roundup(size, BITS_PER_BYTE) / BITS_PER_BYTE;
+	dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					    &dest_data_dma_addr, GFP_KERNEL);
+	if (!dest_data_addr)
+		return -ENOMEM;
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
+	req.dest_data_addr = cpu_to_le64(dest_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	memcpy(buf, dest_data_addr, bytesize);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, dest_data_addr,
+			  dest_data_dma_addr);
+
+	return rc;
+}
+
+static int bnxt_nvm_write(struct bnxt *bp, int nvm_param, int idx,
+			  const void *buf, int size)
+{
+	struct hwrm_nvm_set_variable_input req = {0};
+	dma_addr_t src_data_dma_addr;
+	void *src_data_addr = NULL;
+	int bytesize;
+	int rc;
+
+	bytesize = roundup(size, BITS_PER_BYTE) / BITS_PER_BYTE;
+
+	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					   &src_data_dma_addr, GFP_KERNEL);
+	if (!src_data_addr)
+		return -ENOMEM;
+
+	memcpy(src_data_addr, buf, bytesize);
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_SET_VARIABLE, -1, -1);
+	req.src_data_addr = cpu_to_le64(src_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, src_data_addr,
+			  src_data_dma_addr);
 
-	if (bp->hwrm_spec_code < 0x10803) {
-		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
-		return -ENOTSUPP;
+	return 0;
+}
+
+static int bnxt_dl_perm_config_set(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   enum devlink_perm_config_type type,
+				   union devlink_perm_config_value *value,
+				   bool *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry = NULL;
+	u32 value_int;
+	u32 bytesize;
+	int idx = 0;
+	int ret = 0;
+	u16 val16;
+	u8 val8;
+	int i;
+
+	*restart_reqd = 0;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (param == bnxt_drv_cfgparam_list[i].param) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (!entry)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
 	}
 
+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
+
+	/* Type expected by device may or may not be the same as type passed
+	 * in from devlink API.  So first convert to an interval u32 value,
+	 * then to type needed by device.
+	 */
+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
+		value_int = value->value8;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
+		value_int = value->value16;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
+		value_int = value->value32;
+	} else {
+		/* Unsupported type */
+		return -EINVAL;
+	}
+
+	if (bytesize == 1) {
+		val8 = value_int;
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
+				     entry->bitlength);
+	} else if (bytesize == 2) {
+		val16 = value_int;
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
+				     entry->bitlength);
+	} else {
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
+				     entry->bitlength);
+	}
+
+	/* Restart required for all nvm parameter writes */
+	*restart_reqd = 1;
+
+	return ret;
+}
+
+static int bnxt_dl_perm_config_get(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   enum devlink_perm_config_type type,
+				   union devlink_perm_config_value *value)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry = NULL;
+	u32 value_int;
+	u32 bytesize;
+	int idx = 0;
+	int err = 0;
+	void *data;
+	u16 val16;
+	u8 val8;
+	int i;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (param == bnxt_drv_cfgparam_list[i].param) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (!entry)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
+	}
+
+	/* Allocate space, retrieve value, and copy to result */
+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	err = bnxt_nvm_read(bp, entry->nvm_param, idx, data, entry->bitlength);
+	if (err)
+		goto err_data;
+
+	/* Type returned by device may or may not be the same as type expected
+	 * by devlink API.  So first convert to an interval u32 value, then to
+	 * type expected by devlink.
+	 */
+	if (bytesize == 1) {
+		memcpy(&val8, data, sizeof(val8));
+		value_int = val8;
+	} else if (bytesize == 2) {
+		memcpy(&val16, data, sizeof(val16));
+		value_int = val16;
+	} else {
+		memcpy(&value_int, data, sizeof(value_int));
+	}
+
+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
+		value->value8 = value_int;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
+		value->value16 = value_int;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
+		value->value32 = value_int;
+	} else {
+		/* Unsupported type */
+		err = -EINVAL;
+	}
+
+err_data:
+	kfree(data);
+
+	return err;
+}
+
+static struct devlink_ops bnxt_dl_ops = {
+	.perm_config_get = bnxt_dl_perm_config_get,
+	.perm_config_set = bnxt_dl_perm_config_set,
+};
+
+int bnxt_dl_register(struct bnxt *bp)
+{
+	struct devlink *dl;
+	int rc;
+
+#ifdef CONFIG_BNXT_SRIOV
+	/* Add switchdev eswitch mode setting if SRIOV supported */
+	if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
+	    bp->hwrm_spec_code >= 0x10800) {
+		bnxt_dl_ops.eswitch_mode_set = bnxt_dl_eswitch_mode_set;
+		bnxt_dl_ops.eswitch_mode_get = bnxt_dl_eswitch_mode_get;
+	} else
+#endif /* CONFIG_BNXT_SRIOV */
+		netdev_warn(bp->dev, "SR-IOV E-Switch SWITCHDEV mode not supported.\n");
+
 	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
 	if (!dl) {
 		netdev_warn(bp->dev, "devlink_alloc failed");
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index e92a35d..d843a81 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -10,6 +10,23 @@ 
 #ifndef BNXT_DEVLINK_H
 #define BNXT_DEVLINK_H
 
+#define BNXT_DRV_PF 1
+#define BNXT_DRV_VF 2
+
+enum bnxt_drv_appl {
+	BNXT_DRV_APPL_SHARED,
+	BNXT_DRV_APPL_PORT,
+	BNXT_DRV_APPL_FUNCTION
+};
+
+struct bnxt_drv_cfgparam {
+	enum devlink_perm_config_param	param;
+	u8			func; /* BNXT_DRV_PF | BNXT_DRV_VF */
+	enum bnxt_drv_appl	appl; /* applicability (shared, func, port) */
+	u32			bitlength; /* length, in bits */
+	u32			nvm_param;
+};
+
 /* Struct to hold housekeeping info needed by devlink interface */
 struct bnxt_dl {
 	struct bnxt *bp;	/* back ptr to the controlling dev */