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

From: Jakub Kicinski
Date: Tue Apr 21 2020 - 14:21:33 EST


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);
> +}