linux-kernel - Re: [PATCH net-next 2/3] hinic: add sriov feature support

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]

Message-ID: <20200421112121.2f0ddf30@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 21 Apr 2020 11:21:29 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Luo bin <luobin9@...wei.com>
Cc:     <davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>, <luoxianjun@...wei.com>,
        <yin.yinshi@...wei.com>, <cloud.wangxiaoyun@...wei.com>
Subject: Re: [PATCH net-next 2/3] hinic: add sriov feature support

On Tue, 21 Apr 2020 04:56:34 +0000 Luo bin wrote:
> +int hinic_pci_sriov_disable(struct pci_dev *pdev)
> +{
> +	struct hinic_sriov_info *sriov_info;
> +	u16 tmp_vfs;
> +
> +	sriov_info = hinic_get_sriov_info_by_pcidev(pdev);
> +	/* if SR-IOV is already disabled then nothing will be done */
> +	if (!sriov_info->sriov_enabled)
> +		return 0;

Can't happen see below.

> +	if (test_and_set_bit(HINIC_SRIOV_DISABLE, &sriov_info->state)) {
> +		dev_err(&pdev->dev,
> +			"SR-IOV disable in process, please wait");
> +		return -EPERM;
> +	}

Hm. I don't understand why you need these bit locks.

> +	/* If our VFs are assigned we cannot shut down SR-IOV
> +	 * without causing issues, so just leave the hardware
> +	 * available but disabled
> +	 */
> +	if (pci_vfs_assigned(sriov_info->pdev)) {
> +		clear_bit(HINIC_SRIOV_DISABLE, &sriov_info->state);
> +		dev_warn(&pdev->dev, "Unloading driver while VFs are assigned - VFs will not be deallocated\n");
> +		return -EPERM;
> +	}
> +	sriov_info->sriov_enabled = false;
> +
> +	/* disable iov and allow time for transactions to clear */
> +	pci_disable_sriov(sriov_info->pdev);
> +
> +	tmp_vfs = (u16)sriov_info->num_vfs;
> +	sriov_info->num_vfs = 0;
> +	hinic_deinit_vf_hw(sriov_info, OS_VF_ID_TO_HW(0),
> +			   OS_VF_ID_TO_HW(tmp_vfs - 1));
> +
> +	clear_bit(HINIC_SRIOV_DISABLE, &sriov_info->state);
> +
> +	return 0;
> +}
> +
> +int hinic_pci_sriov_enable(struct pci_dev *pdev, int num_vfs)
> +{
> +	struct hinic_sriov_info *sriov_info;
> +	int pre_existing_vfs = 0;
> +	int err = 0;
> +
> +	sriov_info = hinic_get_sriov_info_by_pcidev(pdev);
> +
> +	if (test_and_set_bit(HINIC_SRIOV_ENABLE, &sriov_info->state)) {
> +		dev_err(&pdev->dev,
> +			"SR-IOV enable in process, please wait, num_vfs %d\n",
> +			num_vfs);
> +		return -EPERM;
> +	}

This should never happen, PCI core code will prevent SR-IOV from being
enabled twice in a row, and concurrently. See sriov_numvfs_store().

> +	pre_existing_vfs = pci_num_vf(sriov_info->pdev);
> +
> +	if (num_vfs > pci_sriov_get_totalvfs(sriov_info->pdev)) {
> +		clear_bit(HINIC_SRIOV_ENABLE, &sriov_info->state);
> +		return -ERANGE;
> +	}

Again, can't happen.

> +	if (pre_existing_vfs && pre_existing_vfs != num_vfs) {
> +		err = hinic_pci_sriov_disable(sriov_info->pdev);
> +		if (err) {
> +			clear_bit(HINIC_SRIOV_ENABLE, &sriov_info->state);
> +			return err;
> +		}

And this.

> +	} else if (pre_existing_vfs == num_vfs) {

Or this.

> +		clear_bit(HINIC_SRIOV_ENABLE, &sriov_info->state);
> +		return num_vfs;
> +	}
> +
> +	err = pci_enable_sriov(sriov_info->pdev, num_vfs);
> +	if (err) {
> +		dev_err(&pdev->dev,
> +			"Failed to enable SR-IOV, error %d\n", err);
> +		clear_bit(HINIC_SRIOV_ENABLE, &sriov_info->state);
> +		return err;
> +	}
> +
> +	sriov_info->sriov_enabled = true;
> +	sriov_info->num_vfs = num_vfs;
> +	clear_bit(HINIC_SRIOV_ENABLE, &sriov_info->state);
> +
> +	return num_vfs;
> +}
> +
> +int hinic_pci_sriov_configure(struct pci_dev *dev, int num_vfs)
> +{
> +	struct hinic_sriov_info *sriov_info;
> +
> +	sriov_info = hinic_get_sriov_info_by_pcidev(dev);
> +
> +	if (test_bit(HINIC_FUNC_REMOVE, &sriov_info->state))
> +		return -EFAULT;

I don't think EFAULT is not a correct error code here. Use EBUSY, or
ENODEV?

> +	if (!num_vfs)
> +		return hinic_pci_sriov_disable(dev);
> +	else
> +		return hinic_pci_sriov_enable(dev, num_vfs);
> +}