Re: [PATCH] PCI: handle pci_sriov_set_totalvfs(dev, 0)

[Edward Cree <ecree@solarflare.com>]

On 31/07/14 17:40, Alexander Duyck wrote:
> If this PF-IOV mode is enabled what would be the layout of the PF
> devices?  It seems like you should be able to scan for multiple PFs all
> showing up on the same bus with a certain stride if you wanted to detect it.
Alas, it's (once again) more complicated than that.  There's another
mode, NIC partitioning, which has multiple PFs per port but connected
with a VLAN aggregator rather than a v-switch, which means that each PF
then can support VFs (the firmware limitation is that v-switches can't
be stacked).
So having multiple PFs on the port doesn't necessarily mean we can't do
SR-IOV.

> Thanks for pointing that out.  It seems like that is a bug.  We should
> probably be checking for dev->sriov, not dev->is_physfn before calling
> sriov_release.
>
> I might see about submitting a patch to address that.
Unfortunately, that won't work because dev->sriov is in a union with
dev->physfn (presumably on the grounds that if you have an associated
PF, you're a VF, so you can't have VFs of your own).
So I think the test in pci_iov_release is correct, and anyone changing
dev->is_physfn after the PCI device has been set up is in the wrong.

>> I'm also unconvinced that having !(dev->is_physfn || dev->is_virtfn) is
>> a valid state; but maybe I'm misunderstanding and "is_physfn" doesn't
>> actually mean "is a physical function", but rather "has SR-IOV capability".
> is_physfn is typically used to indicate the device is capable of acting
> as a physical function.  So a non-IOV device will not set either
> is_physfn or is_virtfn.
Ok, well I think a comment to that effect in struct pci_dev might be a
good idea.
>>> In addition this solution would
>>> also resolve the fact that the driver wouldn't actually have to be
>>> loaded for it to work so if someone were to load a driver that didn't
>>> contain the fix they would be blocked from enabling SR-IOV as well.
>> The current driver fails to probe the PF because it assumes the vswitch
>> creation failure is fatal.  There should never be a driver that knows it
>> can live without the vswitch but doesn't know that that breaks SR-IOV.
>>
>> -Edward
> Is the vswitch a hard requirement for something other than SR-IOV?  If
> not then maybe you should consider modifying the driver so it simply
> disabled SR-IOV if you cannot allocate the vswitch instead of blocking
> probe.
>
That's exactly what I'm trying to do with pci_sriov_set_totalvfs(dev,
0); that's the whole point.
> P.S.  You should really consider changing your email signature.  The bit
> about "you may not use, copy or disclose to anyone this message" kind of
> goes against the whole point of submitting patches to an open source
> project.
>
I know; it's automatically appended by our mail server to all outgoing
mail.  I've already raised this with our IT department but they haven't
answered yet :(

-Edward
The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.