On Sun, 08 May 2011 11:54:32 -0700
Yinghai Lu [off-list ref] wrote:
Need to use it in _e1000e_disable_aspm.
when aer happens,
pci_walk_bus already have down_read(&pci_bus_sem)...
then report_slot_reset
==> e1000_io_slot_reset
==> e1000e_disable_aspm
==> pci_disable_link_state...
We can not use pci_disable_link_state, and it will try to hold pci_bus_sem again.
Try to have __pci_disable_link_state that will not need to hold pci_bus_sem.
What about the other callers of e1000e_disable_aspm? Do they already
have the lock held or is it just reset that needs the already locked
version?
pci_disable_link_state_locked would be more descriptive and would match
other usage in the kernel.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
From: Yinghai Lu <yinghai@kernel.org> Date: 2011-05-11 20:24:23
On 05/09/2011 02:35 PM, Jesse Barnes wrote:
On Sun, 08 May 2011 11:54:32 -0700
Yinghai Lu [off-list ref] wrote:
quoted
Need to use it in _e1000e_disable_aspm.
when aer happens,
pci_walk_bus already have down_read(&pci_bus_sem)...
then report_slot_reset
==> e1000_io_slot_reset
==> e1000e_disable_aspm
==> pci_disable_link_state...
We can not use pci_disable_link_state, and it will try to hold pci_bus_sem again.
Try to have __pci_disable_link_state that will not need to hold pci_bus_sem.
What about the other callers of e1000e_disable_aspm? Do they already
have the lock held or is it just reset that needs the already locked
version?
yes.
there is another version when aspm is not defined. and it does not use any lock.
#ifdef CONFIG_PCIEASPM
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
pci_disable_link_state(pdev, state);
}
#else
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
int pos;
u16 reg16;
/*
* Both device and parent should have the same ASPM setting.
* Disable ASPM in downstream component first and then upstream.
*/
pos = pci_pcie_cap(pdev);
pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, ®16);
reg16 &= ~state;
pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
if (!pdev->bus->self)
return;
pos = pci_pcie_cap(pdev->bus->self);
pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, ®16);
reg16 &= ~state;
pci_write_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, reg16);
}
#endif
pci_disable_link_state_locked would be more descriptive and would match
other usage in the kernel.
ok.
Thanks
Yinghai
------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
On Wed, 11 May 2011 13:23:21 -0700
Yinghai Lu [off-list ref] wrote:
On 05/09/2011 02:35 PM, Jesse Barnes wrote:
quoted
On Sun, 08 May 2011 11:54:32 -0700
Yinghai Lu [off-list ref] wrote:
quoted
Need to use it in _e1000e_disable_aspm.
when aer happens,
pci_walk_bus already have down_read(&pci_bus_sem)...
then report_slot_reset
==> e1000_io_slot_reset
==> e1000e_disable_aspm
==> pci_disable_link_state...
We can not use pci_disable_link_state, and it will try to hold pci_bus_sem again.
Try to have __pci_disable_link_state that will not need to hold pci_bus_sem.
What about the other callers of e1000e_disable_aspm? Do they already
have the lock held or is it just reset that needs the already locked
version?
yes.
there is another version when aspm is not defined. and it does not use any lock.
#ifdef CONFIG_PCIEASPM
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
pci_disable_link_state(pdev, state);
}
#else
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
int pos;
u16 reg16;
/*
* Both device and parent should have the same ASPM setting.
* Disable ASPM in downstream component first and then upstream.
*/
pos = pci_pcie_cap(pdev);
pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, ®16);
reg16 &= ~state;
pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
if (!pdev->bus->self)
return;
pos = pci_pcie_cap(pdev->bus->self);
pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, ®16);
reg16 &= ~state;
pci_write_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, reg16);
}
#endif
No, I mean __e1000e_disable_aspm is called from several spots:
*** drivers/net/e1000e/82571.c:
e1000_get_variants_82571[435] e1000e_disable_aspm(adapter->pdev, PCIE_LINK_STATE_L0S);
*** drivers/net/e1000e/netdev.c:
e1000_change_mtu[5027] e1000e_disable_aspm(adapter->pdev, PCIE_LINK_STATE_L1);
__e1000_resume[5402] e1000e_disable_aspm(pdev, PCIE_LINK_STATE_L1);
e1000_io_slot_reset[5650] e1000e_disable_aspm(pdev, PCIE_LINK_STATE_L1);
e1000_probe[5797] e1000e_disable_aspm(pdev, PCIE_LINK_STATE_L1);
Are all of them safe for the unlocked version of ASPM disable?
--
Jesse Barnes, Intel Open Source Technology Center
From: Yinghai Lu <yinghai@kernel.org> Date: 2011-05-13 00:00:05
On 05/12/2011 04:32 PM, Jesse Barnes wrote:
On Wed, 11 May 2011 13:23:21 -0700
Yinghai Lu [off-list ref] wrote:
quoted
On 05/09/2011 02:35 PM, Jesse Barnes wrote:
quoted
On Sun, 08 May 2011 11:54:32 -0700
Yinghai Lu [off-list ref] wrote:
quoted
Need to use it in _e1000e_disable_aspm.
when aer happens,
pci_walk_bus already have down_read(&pci_bus_sem)...
then report_slot_reset
==> e1000_io_slot_reset
==> e1000e_disable_aspm
==> pci_disable_link_state...
We can not use pci_disable_link_state, and it will try to hold pci_bus_sem again.
Try to have __pci_disable_link_state that will not need to hold pci_bus_sem.
What about the other callers of e1000e_disable_aspm? Do they already
have the lock held or is it just reset that needs the already locked
version?
yes.
there is another version when aspm is not defined. and it does not use any lock.
#ifdef CONFIG_PCIEASPM
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
pci_disable_link_state(pdev, state);
}
#else
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
int pos;
u16 reg16;
/*
* Both device and parent should have the same ASPM setting.
* Disable ASPM in downstream component first and then upstream.
*/
pos = pci_pcie_cap(pdev);
pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, ®16);
reg16 &= ~state;
pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
if (!pdev->bus->self)
return;
pos = pci_pcie_cap(pdev->bus->self);
pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, ®16);
reg16 &= ~state;
pci_write_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, reg16);
}
#endif
No, I mean __e1000e_disable_aspm is called from several spots:
*** drivers/net/e1000e/82571.c:
e1000_get_variants_82571[435] e1000e_disable_aspm(adapter->pdev, PCIE_LINK_STATE_L0S);
*** drivers/net/e1000e/netdev.c:
e1000_change_mtu[5027] e1000e_disable_aspm(adapter->pdev, PCIE_LINK_STATE_L1);
__e1000_resume[5402] e1000e_disable_aspm(pdev, PCIE_LINK_STATE_L1);
e1000_io_slot_reset[5650] e1000e_disable_aspm(pdev, PCIE_LINK_STATE_L1);
e1000_probe[5797] e1000e_disable_aspm(pdev, PCIE_LINK_STATE_L1);
Are all of them safe for the unlocked version of ASPM disable?
yes, there are two version __e1000e_disable_aspm(), one is when aspm support is compiled in, and another one is not.
the one without aspm compiled does not use pci_bus_sem in it self...
So I assume another path should not use pci_bus_sem in the function itself.
Yinghai
------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired