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

["luobin (L)" <luobin9@huawei.com>]

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