Re: return value for "if (!dev->is_physfn)"

[Don Dutile <ddutile@redhat.com>]

On 07/26/2013 12:43 PM, Alexander Duyck wrote:
> On 07/26/2013 02:55 AM, Stefan Assmann wrote:
>> Looking at drivers/pci/iov.c I see at least 3 different return values
>> for if (!dev->is_physfn).
>>
>> sriov_enable() and pci_enable_sriov()
>> [...]
>> 	if (!dev->is_physfn)
>> 		return -ENODEV;
>> pci_num_vf() and pci_vfs_assigned()
>> [...]
>> 	if (!dev->is_physfn)
>> 		return 0;
>> pci_sriov_set_totalvfs() and pci_sriov_get_totalvfs()
>> [...]
>> 	if (!dev->is_physfn)
>> 		return -EINVAL;
>>
>> I'd like to make this consistently return one of the above. Question is,
>> which one should it be? I'd lean towards -ENODEV, other opinions?
>>
>>    Stefan
>
> It all depends on how the results are meant to be interpreted.
>
> In the case of pci_num_vf and pci_vfs_assigned the return of 0 is
> preferred since there are no VFs if the device is not a physical
> function.  I really think pci_sriov_get_totalvfs should probably just
> return 0 as well since it is simply supposed to return the total number
> of VFs supported on the device and 0 would be valid in this case.  Also
> that way the behavior is consistent if CONFIG_PCI_IOV is enabled or
> disabled in the kernel.
>
+1. returning enosys hangs the caller (echo/cat of sysfs) IIRC.

> As for the rest my preference is ENOSYS rather than EINVAL or ENODEV.
> The issue is that the SR-IOV functionality is not implemented for the
> device or in the OS when we return the error so it would make sense to
> return that as an error code in these cases.
>
> Thanks,
>
> Alex
>
+1
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html