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 <[email protected]>
>>>>
>>>>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 <[email protected]>
>>>>Signed-off-by: Bjorn Helgaas <[email protected]>
>>>>---
>>>>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
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Bjorn Helgaas <[email protected]>
>>>>> ---
>>>>> 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.
- Alex
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 <[email protected]>
>>>>>>
>>>>>>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 <[email protected]>
>>>>>>Signed-off-by: Bjorn Helgaas <[email protected]>
>>>>>>---
>>>>>>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