Thread (23 messages) 23 messages, 5 authors, 2017-10-25

RE: [PATCH net-next v3 06/10] bnxt: Add devlink support for config get/set

From: Yuval Mintz <hidden>
Date: 2017-10-25 05:56:01

+static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
+			 void *buf, int size)
+{
+	struct hwrm_nvm_get_variable_input req = {0};
+	dma_addr_t dest_data_dma_addr;
+	void *dest_data_addr = NULL;
+	int bytesize;
+	int rc;
+
+	bytesize = (size + 7) / BITS_PER_BYTE;
roundup?

..

+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 = (size + 7) / BITS_PER_BYTE;
Likewise
+
+	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					   &src_data_dma_addr,
GFP_KERNEL);
+	if (!src_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
Won't you see an oom? Why do you need the print?
+static int bnxt_dl_perm_config_set(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   u8 type, void *value, u8 *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry;
+	int idx = 0;
+	int ret = 0;
+	u32 bytesize;
+	u32 val32;
+	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 (i == BNXT_NUM_DRV_CFGPARAM)
+		return -EINVAL;
+
Looks cleaner to check whether entry is set instead
...
+	bytesize = (entry->bitlength + 7) / BITS_PER_BYTE;
Roundup?

...
+		if (bytesize == 1) {
+			val8 = val32;
Don't you need explicit castings for these kind of assignments
to prevent warnings?
+			ret = bnxt_nvm_write(bp, entry->nvm_param, idx,
&val8,
+					     entry->bitlength);
+		} else if (bytesize == 2) {
+			val16 = val32;
+			ret = bnxt_nvm_write(bp, entry->nvm_param, idx,
&val16,
+					     entry->bitlength);
+		} else {
+			ret = bnxt_nvm_write(bp, entry->nvm_param, idx,
&val32,
+					     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,
+				   u8 type, void *value)
+{
Same comments as for the setter
...
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
-
-	if (bp->hwrm_spec_code < 0x10800) {
+	if ((!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV)) ||
+	    bp->hwrm_spec_code < 0x10800) {
+		/* eswitch switchdev mode not supported */
+		bnxt_dl_ops.eswitch_mode_set = NULL;
+		bnxt_dl_ops.eswitch_mode_get = NULL;
Why would you need to tie this interface to the presence of SRIOV in PCIe?
Also, Assuming the ability to disable sriov in #2 would cause this capability
not to be exposed after reboot, isn't this a one-way ticket?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help