pci_sriov_set_totalvfs again

Calling pci_sriov_set_totalvfs(dev, 0) has no effect, because pci_sriov_get_totalvfs ignores dev->sriov->driver_max_VFs if it's 0, as that is used as the 'not set' value. So, three questions:
a) is this a bug?
b) if not, should the comment on pci_sriov_set_totalvfs mention that passing numvfs=0 will be interpreted as numvfs=dev->sriov->total_VFs?
c) is there a better way of indicating "current configuration doesn't support VFs" rather than calling set_totalvfs(0)?

Thanks,
-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.

Quoted 10 lines · Edward Cree

Calling pci_sriov_set_totalvfs(dev, 0) has no effect, because pci_sriov_get_totalvfs ignores dev->sriov->driver_max_VFs if it's 0, as that is used as the 'not set' value.
So, three questions:
a) is this a bug?
b) if not, should the comment on pci_sriov_set_totalvfs mention that passing numvfs=0 will be interpreted as numvfs=dev->sriov->total_VFs?
c) is there a better way of indicating "current configuration doesn't support VFs" rather than calling set_totalvfs(0)?

Quoted 2 lines

Thanks,
-Edward

The file shouldn't exist if the device doesn't provide an SRIOV capability.
If it does, and it's not supported, then add a patch in quirks.c.

Quoted 11 lines · Don Dutile

Quoted 10 lines · Edward Cree

Calling pci_sriov_set_totalvfs(dev, 0) has no effect, because pci_sriov_get_totalvfs ignores dev->sriov->driver_max_VFs if it's 0, as that is used as the 'not set' value.
So, three questions:
a) is this a bug?
b) if not, should the comment on pci_sriov_set_totalvfs mention that passing numvfs=0 will be interpreted as numvfs=dev->sriov->total_VFs?
c) is there a better way of indicating "current configuration doesn't support VFs" rather than calling set_totalvfs(0)?

Quoted 8 lines

Quoted 2 lines

Thanks,
-Edward

The file shouldn't exist if the device doesn't provide an SRIOV capability.
If it does, and it's not supported, then add a patch in quirks.c.

I don't know much about quirks, but I'm not sure they're the answer here, as it's not quite as simple as "driver doesn't support it". It's a firmware / configuration issue, that if the device (it's a NIC) is configured a certain way [1], the VFs - while appearing fine from a PCI perspective - don't actually work (they can't pass traffic). We can't detect this misconfiguration until PF probe time, and we need a way to report that the VFs aren't usable.

Can quirks handle this?

-Edward

[1] SFC9120-based NICs support multiple PFs per port and these can be used as a kind of "poor-man's SR-IOV" (we're calling it 'PF-IOV') by placing the firmware v-switch below the PFs. However, this then precludes adding a v-switch above the PF to direct VF traffic, meaning that VFs are useless in this configuration. Consequently, our configuration tools won't allow VFs and PF-IOV to be enabled simultaneously, but bugs or corruption could cause this to happen.

My $.02 on the issue would be to simply have sriov_configure return an error indicating the resources are not available if you have the PF-IOV mode enabled it is consuming the VF v-switch resources. Thanks, Alex

Make a driver_max_VFs of 0 mean 0, instead of total_VFs as previously. Initialise driver_max_VFs to total_VFs instead of 0.

Signed-off-by Edward Cree <[email protected]>

---
Only existing callers are ixgbe and igb, and neither passes 0, so this doesn't break existing users.

---
 drivers/pci/iov.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/pci/iov.c +1 -1

index cb6f247..fb51467 100644
@@ -381,7 +381,7 @@ found:
        iov->pos = pos;
        iov->nres = nres;
        iov->ctrl = ctrl;
-       iov->total_VFs = total;
+       iov->driver_max_VFs = iov->total_VFs = total;
        iov->offset = offset;
        iov->stride = stride;
        iov->pgsz = pgsz;

However if you are wanting to use this as a way to disable SR-IOV for a device I wouldn't recommend it. Based on what you have described I would suggest dropping the fix in drivers/pci/quirks that would clear dev->is_physfn if you are in the PF-IOV mode. That way the SR-IOV functionality should be fully disabled.

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.

Quoted 5 lines · Alexander Duyck

However if you are wanting to use this as a way to disable SR-IOV for a device I wouldn't recommend it. Based on what you have described I would suggest dropping the fix in drivers/pci/quirks that would clear dev->is_physfn if you are in the PF-IOV mode. That way the SR-IOV functionality should be fully disabled.

I still don't see how this can go in drivers/pci/quirks, because PF-IOV mode can only be detected at driver probe time. The way we find out that the NIC is in PF-IOV mode is that a request to the MC (the Management CPU on the card) to create a vswitch fails. But maybe I'm wrong about what quirks can do - is there any relevant documentation?

Clearing dev->is_physfn in the driver probe routine isn't safe:
dev->sriov memory can get leaked, for example, as sriov_init will get called at device add, but sriov_release won't be called at removal.

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".

Quoted 4 lines

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

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.

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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 1 line

-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.

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.

Quoted 3 lines · Alexander Duyck

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.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

-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.

Quoted 5 lines

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. --
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Edward Cree

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.

Ed,
It is fairly obvious that your PCIe device operates in very unexpected, non-std modes.

Ignore twiddling total-vfs, and just have the driver fail sriov configuration when they can operate, print a warning why, and let's not create
complicated/convoluted hacks dependent on the use of various assumptions/uses
of pdev flags.

Ideally, when the device is configured in different modes, the SRIOV cap structure
should be modified so the pci sriov code doesn't try to act or reflect the
non-reality the device is in.

Quoted 5 lines · Don Dutile

Ideally, when the device is configured in different modes, the SRIOV cap structure
should be modified so the pci sriov code doesn't try to act or reflect the
non-reality the device is in.

That would be much nicer of course, and I'll ask our firmware team if that's possible. But I don't think it will be.

Quoted 8 lines · Edward Cree

Quoted 5 lines · Don Dutile

Ideally, when the device is configured in different modes, the SRIOV cap structure
should be modified so the pci sriov code doesn't try to act or reflect the
non-reality the device is in.

That would be much nicer of course, and I'll ask our firmware team if that's possible. But I don't think it will be.

As I feared, it's not practical for the device to do this. (Unfortunately, the IMEM (instruction memory) on the device is rather small and the firmware team are struggling to fit all the required features into it.)
However, I also found out that in principle, the device _is_ capable of supporting VFs in PF-IOV mode, by attaching them directly to the underlying v-switch. The firmware may or may not already do this; the person who's likely to know this isn't in today. The driver at present doesn't support it because it assumes it always needs to attach VFs to its own v-switch, but it could (again, in principle) be taught otherwise.

So at this point it becomes a policy question of whether we want to support this, and our opinion is that this isn't a valid use-case (as the only time you'd want PF-IOV is if your system doesn't support VFs) and thus shouldn't be supported (as it creates extra testing and maintenance work). The driver should probe the device, detect PF-IOV mode, recognise that it (the driver) doesn't have support for VFs in that mode, and set totalvfs to 0.
Others may demur, of course, but that's likely to be decided when sending the driver patches to davem. It's my belief that, uncertainty about my use case notwithstanding, pci_sriov_set_totalvfs(dev, 0) should be a valid operation with the semantics I've implemented in my patch.

-Edward

How is it you get yourself into the PF-IOV mode? Is this something that is stored in the NVM of the adapter, or is it something where the driver has to issue a request for it? Also does each PF advertise SR-IOV support in PF-IOV mode or only function 0?

Modifying the total VFs and/or the initial VFs in the configuration space is not practical. You would likely run into issues as one of these fields is read at probe time and the other is read at SR-IOV enablement and if the two differ you cannot enable SR-IOV.

I'm not sure what davem has to do with any of this. We are discussing PCI at this point, not the network drivers. As such Dave won't be the one accepting the pci_sriov_set_totalvfs changes you proposed.

rather than trying to change the SR-IOV API to match your use case.

Quoted 32 lines · Edward Cree

Quoted 8 lines · Edward Cree

Quoted 5 lines · Don Dutile

Ideally, when the device is configured in different modes, the SRIOV cap structure
should be modified so the pci sriov code doesn't try to act or reflect the
non-reality the device is in.

That would be much nicer of course, and I'll ask our firmware team if that's possible. But I don't think it will be.

As I feared, it's not practical for the device to do this. (Unfortunately, the IMEM (instruction memory) on the device is rather small and the firmware team are struggling to fit all the required features into it.)
However, I also found out that in principle, the device _is_ capable of supporting VFs in PF-IOV mode, by attaching them directly to the underlying v-switch. The firmware may or may not already do this; the person who's likely to know this isn't in today. The driver at present doesn't support it because it assumes it always needs to attach VFs to its own v-switch, but it could (again, in principle) be taught otherwise.

So at this point it becomes a policy question of whether we want to support this, and our opinion is that this isn't a valid use-case (as the only time you'd want PF-IOV is if your system doesn't support VFs) and thus shouldn't be supported (as it creates extra testing and maintenance work). The driver should probe the device, detect PF-IOV mode, recognise that it (the driver) doesn't have support for VFs in that mode, and set totalvfs to 0.
Others may demur, of course, but that's likely to be decided when sending the driver patches to davem. It's my belief that, uncertainty about my use case notwithstanding, pci_sriov_set_totalvfs(dev, 0) should be a valid operation with the semantics I've implemented in my patch.

Quoted 1 line

-Edward

How is it you get yourself into the PF-IOV mode? Is this something that is stored in the NVM of the adapter, or is it something where the driver has to issue a request for it? Also does each PF advertise SR-IOV support in PF-IOV mode or only function 0?

Modifying the total VFs and/or the initial VFs in the configuration space is not practical. You would likely run into issues as one of these fields is read at probe time and the other is read at SR-IOV enablement and if the two differ you cannot enable SR-IOV.

I'm not sure what davem has to do with any of this. We are discussing PCI at this point, not the network drivers. As such Dave won't be the one accepting the pci_sriov_set_totalvfs changes you proposed. If you are just wanting to get a patch through Dave for this you would be much better off just returning an error on sriov_configure if the vswitch isn't available rather than trying to change the SR-IOV API to match your use case.

Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Alexander Duyck

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.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

-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.

It sounds reasonable. My only real concern is the fact that the error reported when attempting to enable the interface will be a ERANGE when really a more appropriate error return might be something like a EBUSY since it is the vswitch that is occupied.

You might want to take a look at modifying sriov_numvfs_store so that if the only option is 0 you return EBUSY for all non-zero values requested.
This way it will be more in sync with how we handle the case of
attempting to update the number of VFs when SR-IOV is already enabled.

Quoted 5 lines · Alexander Duyck

However if you are wanting to use this as a way to disable SR-IOV for a device I wouldn't recommend it. Based on what you have described I would suggest dropping the fix in drivers/pci/quirks that would clear dev->is_physfn if you are in the PF-IOV mode. That way the SR-IOV functionality should be fully disabled.

I still don't see how this can go in drivers/pci/quirks, because PF-IOV mode can only be detected at driver probe time. The way we find out that the NIC is in PF-IOV mode is that a request to the MC (the Management CPU on the card) to create a vswitch fails. But maybe I'm wrong about what quirks can do - is there any relevant documentation?

Clearing dev->is_physfn in the driver probe routine isn't safe:
dev->sriov memory can get leaked, for example, as sriov_init will get called at device add, but sriov_release won't be called at removal.

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".

Quoted 4 lines

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
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. --
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Edward Cree

Clearing dev->is_physfn in the driver probe routine isn't safe:
dev->sriov memory can get leaked, for example, as sriov_init will get called at device add, but sriov_release won't be called at removal.

I think this memory leak bug doesn't exist.

The dev->sriov memory was allocated when pci_device_add() called.
whenever devices
were hot-pluged into system or built-in devices were scanned while booting.

The memory of dev->sriov is to be freed by kobject->kobject_release() when
the device reference counter reaches zero when put_device() was called. for hot-plug, that is pciehp_unconfigure_device()....etc. but not in driver->remove().

     put_device()
        kobject_put()
           kobject_release()
               kobject_cleanup()
                   device_release()
                       dev->release(dev)
                             pci_release_dev()
                                  pci_release_capabilities()
                                     pci_iov_release()
                                         sriov_release(dev);
                                               kfree(dev->sriov);

Quoted 8 lines · Ethan Zhao

Quoted 4 lines · Edward Cree

Clearing dev->is_physfn in the driver probe routine isn't safe:
dev->sriov memory can get leaked, for example, as sriov_init will get called at device add, but sriov_release won't be called at removal.

I think this memory leak bug doesn't exist.

Quoted 8 lines

The dev->sriov memory was allocated when pci_device_add() called.
whenever devices
were hot-pluged into system or built-in devices were scanned while booting.

The memory of dev->sriov is to be freed by kobject->kobject_release() when
the device reference counter reaches zero when put_device() was called. for hot-plug, that is pciehp_unconfigure_device()....etc. but not in driver->remove().

Yes, but the problem is in pci_iov_release(), not how it gets called; its code is
if (dev->is_physfn)

       sriov_release(dev);

So if the device driver clears dev->is_physfn, and later on the device is hot-unplugged, pci_iov_release() won't call sriov_release(). Thus any device driver that changes dev->is_physfn will, if the device is hot-unplugged, leak dev->sriov.

Fortunately a quick grep shows no drivers doing this, so there's no actual bug in existing code. It's just that we can't use this approach to disable SR-IOV from the driver's probe routine.
-Edward

Quoted 11 lines

    put_device()
       kobject_put()
          kobject_release()
              kobject_cleanup()
                  device_release()
                      dev->release(dev)
                            pci_release_dev()
                                 pci_release_capabilities()
                                    pci_iov_release()
                                        sriov_release(dev);
                                              kfree(dev->sriov);

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.

Make a driver_max_VFs of 0 mean 0, instead of total_VFs as previously. Initialise driver_max_VFs to total_VFs instead of 0.

Signed-off-by Edward Cree <[email protected]>

---
Only existing callers are ixgbe and igb, and neither passes 0, so this doesn't break existing users.

---
 drivers/pci/iov.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/pci/iov.c +2 -6

index cb6f247..fb51467 100644
@@ -381,7 +381,7 @@ found:
        iov->pos = pos;
        iov->nres = nres;
        iov->ctrl = ctrl;
-       iov->total_VFs = total;
+       iov->driver_max_VFs = iov->total_VFs = total;
        iov->offset = offset;
        iov->stride = stride;
        iov->pgsz = pgsz;

@@ -686,9 +686,6 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
        if (!dev->is_physfn)
                return 0;
 
-       if (dev->sriov->driver_max_VFs)
-               return dev->sriov->driver_max_VFs;
-
-       return dev->sriov->total_VFs;
+       return dev->sriov->driver_max_VFs;
 }
 EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
--

1.7.11.7
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. --
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Though I like this patch, it shows direct style, but it will change the meaning of a function not mentioned here.

int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)

So far seems no driver call pci_sriov_set_totalvfs() with numvfs = 0 and the patch will change the behavior of the drivers, but it will definitely change the test cases and documents related to IOV.

Quoted 6 lines · Ethan Zhao

Though I like this patch, it shows direct style, but it will change the meaning of a function not mentioned here.

int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)

Am I missing something here, or is that exactly the function mentioned in the patch title / subject line?

Quoted 3 lines

So far seems no driver call pci_sriov_set_totalvfs() with numvfs = 0 and the patch will change the behavior of the drivers, but it will definitely change the test cases and documents related to IOV.

As for test cases and documents, I can't find any in the kernel tree - if I grep for pci_sriov_set_totalvfs, the only mentions I find are:
* The declaration in include/linux/pci.h. Doesn't have any comments.
* The definition in drivers/pci/iov.c. The comment doesn't document the behaviour in the numvfs = 0 case.
* Documentation/ABI/testing/sysfs-bus-pci. Describes the function's purpose "reduce the value read from [the sriov_totalvfs sysfs file]" but doesn't say anything about the numvfs = 0 case.
* the Intel igb and ixgbe ethernet drivers. Both pass nonzero constants for numvfs.
* the Emulex benet driver. This went into Dave Miller's net-next branch on 30/06/2014 (so hasn't hit Linus' or other trees yet), but it looks to me like it might call with numvfs = 0. CCing the author of that patch (Vasundhara Volam) for comments.

-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.

Quoted 5 lines · 在 2014年8月1日，下午8:15，Edward Cree <[email protected]> 写道：

Quoted 3 lines · Ethan Zhao

So far seems no driver call pci_sriov_set_totalvfs() with numvfs = 0 and the patch will change the behavior of the drivers, but it will definitely change the test cases and documents related to IOV.

As for test cases and documents, I can't find any in the kernel tree

I mean some test cases or doc that are out of kernel tree ... such as some Distro, not only Hardware vendors.

Quoted 6 lines · Ethan Zhao

Quoted 5 lines · 在 2014年8月1日，下午8:15，Edward Cree <[email protected]> 写道：

Quoted 3 lines · Ethan Zhao

So far seems no driver call pci_sriov_set_totalvfs() with numvfs = 0 and the patch will change the behavior of the drivers, but it will definitely change the test cases and documents related to IOV.

As for test cases and documents, I can't find any in the kernel tree

Quoted 2 lines

I mean some test cases or doc that are out of kernel tree ... such as some Distro, not only Hardware vendors.

I was under the impression that we don't care about breaking out-of-tree code. Distros should be (and it looks like most are) using the kernel-doc to generate their documentation, and the kernel-doc doesn't say anything about what happens when passing 0.

-Edward

Edward,

The concern isn't what is out there, but what will come of it as a result. The question is should we allow setting the value to 0 to change the behavior from disabling the override to essentially disabling SR-IOV.

If we consider it acceptable to use 0 as a disable value, and there are several people on this thread that don't, the secondary issue of this is the error return for sriov_numvfs_store which hasn't been addressed. Should we still be returning ERANGE if SR-IOV has essentially been disabled, or should we be returning some other error such as EBUSY, ENOMEM, ENODEV, or ENOSYS to indicate that there are no resources available for SR-IOV.

I think you would be much better off just implementing sriov_configure for your PCI driver and placing your own error return there if you cannot allocate a vswitch.