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

[luobin (L)]

These bit locks are mainly used for extendibility, and other opinions are accepted.Thanks for your review.

On 2020/4/22 2:21, Jakub Kicinski wrote:
> 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);
> > +}
> .