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?