On Mon, Nov 02, 2015 at 07:46:24AM -0800, Alexander Duyck wrote: >On 11/02/2015 12:33 AM, Wei Yang wrote: >>On Fri, Oct 30, 2015 at 08:57:17AM -0700, Alexander Duyck wrote: >>>On 10/29/2015 11:00 PM, ethan zhao wrote: >>>>Wei, >>>> >>>>On 2015/10/30 13:14, Wei Yang wrote: >>>>>On Thu, Oct 29, 2015 at 05:23:22PM -0500, Bjorn Helgaas wrote: >>>>>>From: Alexander Duyck <aduyck@xxxxxxxxxxxx> >>>>>> >>>>>>Per sec 3.3.3.1 of the SR-IOV spec, r1.1, we must allow 1.0s after >>>>>>clearing >>>>>>VF Enable before reading any field in the SR-IOV Extended Capability. >>>>>> >>>>>>Wait 1 second before calling pci_iov_set_numvfs(), which reads >>>>>>PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE after it sets >>>>>>PCI_SRIOV_NUM_VF. >>>>>> >>>>>>[bhelgaas: split to separate patch for reviewability, add spec >>>>>>reference] >>>>>>Signed-off-by: Alexander Duyck <aduyck@xxxxxxxxxxxx> >>>>>>Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> >>>>>>--- >>>>>>drivers/pci/iov.c | 2 +- >>>>>>1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c >>>>>>index fada98d..24428d5 100644 >>>>>>--- a/drivers/pci/iov.c >>>>>>+++ b/drivers/pci/iov.c >>>>>>@@ -339,13 +339,13 @@ failed: >>>>>> iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE); >>>>>> pci_cfg_access_lock(dev); >>>>>> pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl); >>>>>>- pci_iov_set_numvfs(dev, 0); >>>>>> ssleep(1); >>>>>> pci_cfg_access_unlock(dev); >>>>>> >>>>>> if (iov->link != dev->devfn) >>>>>> sysfs_remove_link(&dev->dev.kobj, "dep_link"); >>>>>> >>>>>>+ pci_iov_set_numvfs(dev, 0); >>>>>One small question, any specific reason put it here instead of just after >>>>>sleep()? >>>>Agree, pci_iov_set_numvfs(dev, 0) should be put before >>>>pci_cfg_access_unlock(dev) to avoid race, because "NumVFs may only be >>>>written while VF Enable is Clear" >>>We are already guaranteeing that aren't we? I'm assuming there is already >>>code in place here somewhere that prevents us from both enabling and >>>disabling SR-IOV from more than one thread. Otherwise how could we hope to >>>have any sort of consistent state? >>> >>>I'm fine with us being more explicit about it if we want to be, but if we are >>>going to do it we should probably update all 3 spots where we update NumVFs >>>after init instead of just this one. Perhaps it should be a separate patch. >>> >>Yep, I think the statement is met, "NumVFs may only be written while VF Enable >>is Clear". >> >>While in your commit log, the purpose of this patch is to wait 1 second before >>write NumVFs. So I am interesting to know why you move this out of the >>pci_cfg_access_lock. Because it looks better? have better performance? >> >>Actually, this is a question instead of a challenge :-) > >It is because the first call to pci_iov_set_numvfs is done outside of the >pci_cfg_access_lock. This way when I add the clean-up for the bus numbering >failure in patch 7 I don't have to modify as much code either since the write >is already pulled out. > >An added bonus is the code is now much closer to what we have in >sriov_disable which has seen much more use than the exception handling case >for sriov_enable, so it has been more thoroughly tested. Thanks~ I am more comfortable with this change~ > >- Alex -- Richard Yang Help you, Help me -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
