e1000e: don't inadvertently re-set INTX_DISABLE
Subsystems (2)
PATCH 1/1 e1000e: don't inadvertently re-set INTX_DISABLE
Should e1000_test_msi() fail to see an msi interrupt, it attempts to fallback to legacy INTx interrupts. But an error in the code may prevent this from happening correctly.
Before calling e1000_test_msi_interrupt(), e1000_test_msi() disables SERR by clearing the SERR bit from the just read PCI_COMMAND bits as it writes them back out.
Upon return from calling e1000_test_msi_interrupt(), it re-enables SERR by writing out the version of PCI_COMMAND it had previously read.
The problem with this is that e1000_test_msi_interrupt() calls pci_disable_msi(), which eventually ends up in pci_intx(). And because pci_intx() was called with enable set to 1, the INTX_DISABLE bit gets cleared from PCI_COMMAND, which is what we want. But when we get back to e1000_test_msi(), the INTX_DISABLE bit gets inadvertently re-set because of the attempt by e1000_test_msi() to re-enable SERR.
The solution is to have e1000_test_msi() re-read the PCI_COMMAND bits as part of its attempt to re-enable SERR.
During debugging/testing of this issue I found that not all the systems I ran on had the SERR bit set to begin with. And on some of the systems the same could be said for the INTX_DISABLE bit. Needless to say these latter systems didn't have a problem falling back to legacy INTx interrupts with the code as is.
---
I forced the msi interrupt test to fail by commenting out the clearing of FLAG_MSI_TEST_FAILED in e1000_intr_msi_test().
drivers/net/e1000e/netdev.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-)
drivers/net/e1000e/netdev.c +9 -4
@@ -3419,13 +3419,18 @@ static int e1000_test_msi(struct e1000_adapter *adapter)/* disable SERR in case the MSI write causes a master abort */ pci_read_config_word (adapter->pdev,PCI_COMMAND , &pci_cmd); -pci_write_config_word (adapter->pdev,PCI_COMMAND , - pci_cmd & ~PCI_COMMAND_SERR ); +if (pci_cmd &PCI_COMMAND_SERR ) +pci_write_config_word (adapter->pdev,PCI_COMMAND , + pci_cmd & ~PCI_COMMAND_SERR ); err =e1000_test_msi_interrupt (adapter); -/* restore previous setting of command word */ -pci_write_config_word (adapter->pdev,PCI_COMMAND , pci_cmd); +/* re-enable SERR */ +if (pci_cmd &PCI_COMMAND_SERR ) { +pci_read_config_word (adapter->pdev,PCI_COMMAND , &pci_cmd); + pci_cmd |=PCI_COMMAND_SERR ; +pci_write_config_word (adapter->pdev,PCI_COMMAND , pci_cmd); + }/* success ! */ if (!err)
Quoted 30 lines · Dean Nelson
Should
e1000_test_msi()fail to see an msi interrupt, it attempts to fallback to legacy INTx interrupts. But an error in the code may prevent this from happening correctly.Before calling
e1000_test_msi_interrupt(),e1000_test_msi()disables SERR by clearing the SERR bit from the just readPCI_COMMANDbits as it writes them back out.Upon return from calling
e1000_test_msi_interrupt(), it re-enables SERR by writing out the version ofPCI_COMMANDit had previously read.The problem with this is that
e1000_test_msi_interrupt()callspci_disable_msi(), which eventually ends up inpci_intx(). And becausepci_intx()was called with enable set to 1, theINTX_DISABLEbit gets cleared fromPCI_COMMAND, which is what we want. But when we get back toe1000_test_msi(), theINTX_DISABLEbit gets inadvertently re-set because of the attempt bye1000_test_msi()to re-enable SERR.The solution is to have
e1000_test_msi()re-read thePCI_COMMANDbits as part of its attempt to re-enable SERR.During debugging/testing of this issue I found that not all the systems I ran on had the SERR bit set to begin with. And on some of the systems the same could be said for the
INTX_DISABLEbit. Needless to say these latter systems didn't have a problem falling back to legacy INTx interrupts with the code as is.Signed-off-by Dean Nelson <[email protected]>
Quoted 8 lines
---
I forced the msi interrupt test to fail by commenting out the clearing of
FLAG_MSI_TEST_FAILEDine1000_intr_msi_test().drivers/net/e1000e/netdev.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
Thanks Dean, I have added it to my queue.
Cheers,
Jeff