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 :-) >- 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
