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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Oct 2017 05:55:58 +0000
From:   Yuval Mintz <yuvalm@...lanox.com>
To:     Steve Lin <steven.lin1@...adcom.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Jiri Pirko <jiri@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        "linville@...driver.com" <linville@...driver.com>,
        "gospo@...adcom.com" <gospo@...adcom.com>
Subject: RE: [PATCH net-next v3 06/10] bnxt: Add devlink support for config
 get/set

> +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?