* [PATCH] pci, e1000e: Add and use __pci_disable_link_state
@ 2011-05-08 18:54 Yinghai Lu
2011-05-09 21:35 ` Jesse Barnes
0 siblings, 1 reply; 5+ messages in thread
From: Yinghai Lu @ 2011-05-08 18:54 UTC (permalink / raw)
To: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
Don Skidmore, Greg Rose, PJ Waskiewicz, Alex Duyck, John Ronciak,
Jesse Barnes, Rafael J. Wysocki, Kenji Kaneshige,
Matthew Garrett, Naga Chumbalkar
Cc: e1000-devel, netdev, linux-kernel, linux-pci, Andrew Morton
Need to use it in _e1000e_disable_aspm.
Found lock up:
[ 2374.654557] kworker/32:1 D ffff881027f6b0f0 0 6075 2 0x00000000
[ 2374.654816] ffff88503f099a68 0000000000000046 ffff88503f098000 0000000000004000
[ 2374.654837] 00000000001d1ec0 ffff88503f099fd8 00000000001d1ec0 ffff88503f099fd8
[ 2374.654860] 0000000000004000 00000000001d1ec0 ffff88503dcc8000 ffff88503f090000
[ 2374.654880] Call Trace:
[ 2374.654898] [<ffffffff810b1302>] ? __lock_acquired+0x3a/0x224
[ 2374.654914] [<ffffffff81c2b59c>] ? _raw_spin_unlock_irq+0x30/0x36
[ 2374.654925] [<ffffffff810b069d>] ? trace_hardirqs_on_caller+0x1f/0x178
[ 2374.654936] [<ffffffff81c2ab24>] rwsem_down_failed_common+0xd3/0x103
[ 2374.654945] [<ffffffff810b158f>] ? __lock_contended+0x3a/0x2a2
[ 2374.654955] [<ffffffff81c2ab7b>] rwsem_down_read_failed+0x12/0x14
[ 2374.654967] [<ffffffff813371e4>] call_rwsem_down_read_failed+0x14/0x30
[ 2374.654981] [<ffffffff8135df20>] ? pci_disable_link_state+0x5f/0xf5
[ 2374.654990] [<ffffffff81c2a0e6>] ? down_read+0x7e/0x91
[ 2374.654999] [<ffffffff8135df20>] ? pci_disable_link_state+0x5f/0xf5
[ 2374.655008] [<ffffffff8135df20>] pci_disable_link_state+0x5f/0xf5
[ 2374.655024] [<ffffffff81661796>] e1000e_disable_aspm+0x55/0x5a
[ 2374.655037] [<ffffffff816677eb>] e1000_io_slot_reset+0x59/0xea
[ 2374.655048] [<ffffffff8135fe0d>] ? report_mmio_enabled+0x5d/0x5d
[ 2374.655057] [<ffffffff8135fe3b>] report_slot_reset+0x2e/0x5d
[ 2374.655072] [<ffffffff8135369e>] pci_walk_bus+0x8a/0xb7
[ 2374.655081] [<ffffffff8135fe0d>] ? report_mmio_enabled+0x5d/0x5d
[ 2374.655091] [<ffffffff813603be>] broadcast_error_message+0xa4/0xb2
[ 2374.655101] [<ffffffff81352c71>] ? pci_bus_read_config_dword+0x72/0x80
[ 2374.655110] [<ffffffff813606df>] do_recovery+0x9e/0xf9
[ 2374.655120] [<ffffffff81360786>] handle_error_source+0x4c/0x51
[ 2374.655129] [<ffffffff81360974>] aer_isr_one_error+0x1e9/0x21a
[ 2374.655138] [<ffffffff81360a6c>] aer_isr+0xc7/0xcc
[ 2374.655147] [<ffffffff813609a5>] ? aer_isr_one_error+0x21a/0x21a
[ 2374.655159] [<ffffffff81096d9f>] process_one_work+0x237/0x3ec
[ 2374.655168] [<ffffffff81096d10>] ? process_one_work+0x1a8/0x3ec
[ 2374.655178] [<ffffffff8109728d>] worker_thread+0x17c/0x240
[ 2374.655186] [<ffffffff810b0803>] ? trace_hardirqs_on+0xd/0xf
[ 2374.655196] [<ffffffff81097111>] ? manage_workers+0xab/0xab
[ 2374.655209] [<ffffffff8109c8ed>] kthread+0xa0/0xa8
[ 2374.655223] [<ffffffff81c332d4>] kernel_thread_helper+0x4/0x10
[ 2374.655232] [<ffffffff81c2b880>] ? retint_restore_args+0xe/0xe
[ 2374.655243] [<ffffffff8109c84d>] ? __init_kthread_worker+0x5b/0x5b
[ 2374.655252] [<ffffffff81c332d0>] ? gs_change+0xb/0xb
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.
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
drivers/net/e1000e/netdev.c | 2 +-
drivers/pci/pcie/aspm.c | 16 +++++++++++++---
include/linux/pci-aspm.h | 1 +
3 files changed, 15 insertions(+), 4 deletions(-)
Index: linux-2.6/drivers/net/e1000e/netdev.c
===================================================================
--- linux-2.6.orig/drivers/net/e1000e/netdev.c
+++ linux-2.6/drivers/net/e1000e/netdev.c
@@ -5360,7 +5360,7 @@ static void e1000_complete_shutdown(stru
#ifdef CONFIG_PCIEASPM
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
{
- pci_disable_link_state(pdev, state);
+ __pci_disable_link_state(pdev, state);
}
#else
static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
Index: linux-2.6/drivers/pci/pcie/aspm.c
===================================================================
--- linux-2.6.orig/drivers/pci/pcie/aspm.c
+++ linux-2.6/drivers/pci/pcie/aspm.c
@@ -734,7 +734,7 @@ void pcie_aspm_powersave_config_link(str
* pci_disable_link_state - disable pci device's link state, so the link will
* never enter specific states
*/
-void pci_disable_link_state(struct pci_dev *pdev, int state)
+static void ___pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
{
struct pci_dev *parent = pdev->bus->self;
struct pcie_link_state *link;
@@ -747,7 +747,8 @@ void pci_disable_link_state(struct pci_d
if (!parent || !parent->link_state)
return;
- down_read(&pci_bus_sem);
+ if (sem)
+ down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
link = parent->link_state;
if (state & PCIE_LINK_STATE_L0S)
@@ -761,7 +762,16 @@ void pci_disable_link_state(struct pci_d
pcie_set_clkpm(link, 0);
}
mutex_unlock(&aspm_lock);
- up_read(&pci_bus_sem);
+ if (sem)
+ up_read(&pci_bus_sem);
+}
+void __pci_disable_link_state(struct pci_dev *pdev, int state)
+{
+ ___pci_disable_link_state(pdev, state, false);
+}
+void pci_disable_link_state(struct pci_dev *pdev, int state)
+{
+ ___pci_disable_link_state(pdev, state, true);
}
EXPORT_SYMBOL(pci_disable_link_state);
Index: linux-2.6/include/linux/pci-aspm.h
===================================================================
--- linux-2.6.orig/include/linux/pci-aspm.h
+++ linux-2.6/include/linux/pci-aspm.h
@@ -28,6 +28,7 @@ extern void pcie_aspm_exit_link_state(st
extern void pcie_aspm_pm_state_change(struct pci_dev *pdev);
extern void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
extern void pci_disable_link_state(struct pci_dev *pdev, int state);
+extern void __pci_disable_link_state(struct pci_dev *pdev, int state);
extern void pcie_clear_aspm(void);
extern void pcie_no_aspm(void);
#else
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci, e1000e: Add and use __pci_disable_link_state
2011-05-08 18:54 [PATCH] pci, e1000e: Add and use __pci_disable_link_state Yinghai Lu
@ 2011-05-09 21:35 ` Jesse Barnes
2011-05-11 20:23 ` Yinghai Lu
0 siblings, 1 reply; 5+ messages in thread
From: Jesse Barnes @ 2011-05-09 21:35 UTC (permalink / raw)
To: Yinghai Lu
Cc: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
Don Skidmore, Greg Rose, PJ Waskiewicz, Alex Duyck, John Ronciak,
Rafael J. Wysocki, Kenji Kaneshige, Matthew Garrett,
Naga Chumbalkar, e1000-devel, netdev, linux-kernel, linux-pci,
Andrew Morton
On Sun, 08 May 2011 11:54:32 -0700
Yinghai Lu <yinghai@kernel.org> 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?
> +extern void __pci_disable_link_state(struct pci_dev *pdev, int state);
> extern void pcie_clear_aspm(void);
> extern void pcie_no_aspm(void);
> #else
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci, e1000e: Add and use __pci_disable_link_state
2011-05-09 21:35 ` Jesse Barnes
@ 2011-05-11 20:23 ` Yinghai Lu
2011-05-12 23:32 ` Jesse Barnes
0 siblings, 1 reply; 5+ messages in thread
From: Yinghai Lu @ 2011-05-11 20:23 UTC (permalink / raw)
To: Jesse Barnes
Cc: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
Don Skidmore, Greg Rose, PJ Waskiewicz, Alex Duyck, John Ronciak,
Rafael J. Wysocki, Kenji Kaneshige, Matthew Garrett,
Naga Chumbalkar, e1000-devel, netdev, linux-kernel, linux-pci,
Andrew Morton
On 05/09/2011 02:35 PM, Jesse Barnes wrote:
> On Sun, 08 May 2011 11:54:32 -0700
> Yinghai Lu <yinghai@kernel.org> 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?
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
>
>> +extern void __pci_disable_link_state(struct pci_dev *pdev, int state);
>> extern void pcie_clear_aspm(void);
>> extern void pcie_no_aspm(void);
>> #else
>
> pci_disable_link_state_locked would be more descriptive and would match
> other usage in the kernel.
ok.
Thanks
Yinghai
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci, e1000e: Add and use __pci_disable_link_state
2011-05-11 20:23 ` Yinghai Lu
@ 2011-05-12 23:32 ` Jesse Barnes
2011-05-12 23:58 ` Yinghai Lu
0 siblings, 1 reply; 5+ messages in thread
From: Jesse Barnes @ 2011-05-12 23:32 UTC (permalink / raw)
To: Yinghai Lu
Cc: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
Don Skidmore, Greg Rose, PJ Waskiewicz, Alex Duyck, John Ronciak,
Rafael J. Wysocki, Kenji Kaneshige, Matthew Garrett,
Naga Chumbalkar, e1000-devel, netdev, linux-kernel, linux-pci,
Andrew Morton
On Wed, 11 May 2011 13:23:21 -0700
Yinghai Lu <yinghai@kernel.org> wrote:
> On 05/09/2011 02:35 PM, Jesse Barnes wrote:
> > On Sun, 08 May 2011 11:54:32 -0700
> > Yinghai Lu <yinghai@kernel.org> 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?
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci, e1000e: Add and use __pci_disable_link_state
2011-05-12 23:32 ` Jesse Barnes
@ 2011-05-12 23:58 ` Yinghai Lu
0 siblings, 0 replies; 5+ messages in thread
From: Yinghai Lu @ 2011-05-12 23:58 UTC (permalink / raw)
To: Jesse Barnes
Cc: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
Don Skidmore, Greg Rose, PJ Waskiewicz, Alex Duyck, John Ronciak,
Rafael J. Wysocki, Kenji Kaneshige, Matthew Garrett,
Naga Chumbalkar, e1000-devel, netdev, linux-kernel, linux-pci,
Andrew Morton
On 05/12/2011 04:32 PM, Jesse Barnes wrote:
> On Wed, 11 May 2011 13:23:21 -0700
> Yinghai Lu <yinghai@kernel.org> wrote:
>
>> On 05/09/2011 02:35 PM, Jesse Barnes wrote:
>>> On Sun, 08 May 2011 11:54:32 -0700
>>> Yinghai Lu <yinghai@kernel.org> 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?
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-05-13 0:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-08 18:54 [PATCH] pci, e1000e: Add and use __pci_disable_link_state Yinghai Lu
2011-05-09 21:35 ` Jesse Barnes
2011-05-11 20:23 ` Yinghai Lu
2011-05-12 23:32 ` Jesse Barnes
2011-05-12 23:58 ` Yinghai Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).