* [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
@ 2013-11-28 7:35 ` Jeff Kirsher
2013-11-29 20:47 ` David Miller
2013-11-28 7:35 ` [net 2/8] igb: Fixed Wake On LAN support Jeff Kirsher
` (6 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:35 UTC (permalink / raw)
To: davem; +Cc: Carolyn Wyborny, netdev, gospo, sassmann, Jeff Kirsher
From: Carolyn Wyborny <carolyn.wyborny@intel.com>
This patch adds a call to dev_close if the queue reinit fails in order
to make clearer to the user that the device is down.
Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 025e5f4..5a64aa6 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -7846,6 +7846,7 @@ int igb_reinit_queues(struct igb_adapter *adapter)
if (igb_init_interrupt_scheme(adapter, true)) {
dev_err(&pdev->dev, "Unable to allocate memory for queues\n");
+ dev_close(netdev);
return -ENOMEM;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-11-28 7:35 ` [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails Jeff Kirsher
@ 2013-11-29 20:47 ` David Miller
2013-11-30 7:52 ` Jeff Kirsher
2013-12-02 16:55 ` Wyborny, Carolyn
0 siblings, 2 replies; 16+ messages in thread
From: David Miller @ 2013-11-29 20:47 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: carolyn.wyborny, netdev, gospo, sassmann
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 27 Nov 2013 23:35:55 -0800
> From: Carolyn Wyborny <carolyn.wyborny@intel.com>
>
> This patch adds a call to dev_close if the queue reinit fails in order
> to make clearer to the user that the device is down.
>
> Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
This is a very bad approach to this problem.
Users absolutely do not expect their entire interface to go down
simply because an ethtool request cannot be satisfied. This is
extremely poor quality of implementation.
And in this specific case it absolutely is not necessary.
The only thing that can fail is the queue allocation, so make a
function which can preserve the previous configuration if the queue
allocation fails. How about "igb_reinit_interrupt_scheme".
Don't free the q vectors until the very last moment, when you know
that the allocation of the new q vectors has succeeded.
I'm not applying this patch, it needs to be reimplemented more
sanely, using the above suggestions or similar.
Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-11-29 20:47 ` David Miller
@ 2013-11-30 7:52 ` Jeff Kirsher
2013-12-02 16:55 ` Wyborny, Carolyn
1 sibling, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-30 7:52 UTC (permalink / raw)
To: David Miller; +Cc: carolyn.wyborny, netdev, gospo, sassmann
[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]
On Fri, 2013-11-29 at 15:47 -0500, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Wed, 27 Nov 2013 23:35:55 -0800
>
> > From: Carolyn Wyborny <carolyn.wyborny@intel.com>
> >
> > This patch adds a call to dev_close if the queue reinit fails in order
> > to make clearer to the user that the device is down.
> >
> > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> This is a very bad approach to this problem.
>
> Users absolutely do not expect their entire interface to go down
> simply because an ethtool request cannot be satisfied. This is
> extremely poor quality of implementation.
>
> And in this specific case it absolutely is not necessary.
>
> The only thing that can fail is the queue allocation, so make a
> function which can preserve the previous configuration if the queue
> allocation fails. How about "igb_reinit_interrupt_scheme".
>
> Don't free the q vectors until the very last moment, when you know
> that the allocation of the new q vectors has succeeded.
>
> I'm not applying this patch, it needs to be reimplemented more
> sanely, using the above suggestions or similar.
>
> Thanks.
Thanks for the feedback Dave, I will drop this patch from the series so
that Carolyn can re-work the solution.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-11-29 20:47 ` David Miller
2013-11-30 7:52 ` Jeff Kirsher
@ 2013-12-02 16:55 ` Wyborny, Carolyn
2013-12-02 17:18 ` Ben Hutchings
1 sibling, 1 reply; 16+ messages in thread
From: Wyborny, Carolyn @ 2013-12-02 16:55 UTC (permalink / raw)
To: David Miller, Kirsher, Jeffrey T
Cc: netdev, gospo, sassmann, Ben Hutchings (bhutchings@solarflare.com)
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Friday, November 29, 2013 12:48 PM
> To: Kirsher, Jeffrey T
> Cc: Wyborny, Carolyn; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: Re: [net 1/8] igb: Update queue reinit function to call dev_close when
> init of queues fails
>
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Wed, 27 Nov 2013 23:35:55 -0800
>
> > From: Carolyn Wyborny <carolyn.wyborny@intel.com>
> >
> > This patch adds a call to dev_close if the queue reinit fails in order
> > to make clearer to the user that the device is down.
> >
> > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> This is a very bad approach to this problem.
>
> Users absolutely do not expect their entire interface to go down simply because
> an ethtool request cannot be satisfied. This is extremely poor quality of
> implementation.
>
> And in this specific case it absolutely is not necessary.
>
> The only thing that can fail is the queue allocation, so make a function which can
> preserve the previous configuration if the queue allocation fails. How about
> "igb_reinit_interrupt_scheme".
>
> Don't free the q vectors until the very last moment, when you know that the
> allocation of the new q vectors has succeeded.
>
> I'm not applying this patch, it needs to be reimplemented more sanely, using the
> above suggestions or similar.
>
I did this per Ben's suggestion on Laura's original patch. You didn't argue with it at the time. I don't think I misunderstood his suggestion, but if so, I'll rework it.
Carolyn
Carolyn Wyborny
Linux Development
Networking Division
Intel Corporation
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-12-02 16:55 ` Wyborny, Carolyn
@ 2013-12-02 17:18 ` Ben Hutchings
2013-12-02 17:39 ` Wyborny, Carolyn
2013-12-02 18:33 ` David Miller
0 siblings, 2 replies; 16+ messages in thread
From: Ben Hutchings @ 2013-12-02 17:18 UTC (permalink / raw)
To: Wyborny, Carolyn
Cc: David Miller, Kirsher, Jeffrey T, netdev, gospo, sassmann
On Mon, 2013-12-02 at 16:55 +0000, Wyborny, Carolyn wrote:
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Friday, November 29, 2013 12:48 PM
> > To: Kirsher, Jeffrey T
> > Cc: Wyborny, Carolyn; netdev@vger.kernel.org; gospo@redhat.com;
> > sassmann@redhat.com
> > Subject: Re: [net 1/8] igb: Update queue reinit function to call dev_close when
> > init of queues fails
> >
> > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Date: Wed, 27 Nov 2013 23:35:55 -0800
> >
> > > From: Carolyn Wyborny <carolyn.wyborny@intel.com>
> > >
> > > This patch adds a call to dev_close if the queue reinit fails in order
> > > to make clearer to the user that the device is down.
> > >
> > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
> > > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >
> > This is a very bad approach to this problem.
> >
> > Users absolutely do not expect their entire interface to go down simply because
> > an ethtool request cannot be satisfied. This is extremely poor quality of
> > implementation.
> >
> > And in this specific case it absolutely is not necessary.
> >
> > The only thing that can fail is the queue allocation, so make a function which can
> > preserve the previous configuration if the queue allocation fails. How about
> > "igb_reinit_interrupt_scheme".
> >
> > Don't free the q vectors until the very last moment, when you know that the
> > allocation of the new q vectors has succeeded.
> >
> > I'm not applying this patch, it needs to be reimplemented more sanely, using the
> > above suggestions or similar.
> >
>
> I did this per Ben's suggestion on Laura's original patch. You didn't
> argue with it at the time. I don't think I misunderstood his
> suggestion, but if so, I'll rework it.
My concerns were that after a failure where the interface is effectively
down,
- the interface must be in a consistent state where is it safe to
reconfigure the interface again or to unbind the driver, and
- it should be obviously down so the user can then try to bring it up
again.
David is saying that you should implement the reconfiguration in such a
way that you can always roll back to the previous working state in case
of a failure (which would make my concerns moot). This is definitely a
good goal but I'm not convinced that it's always possible.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-12-02 17:18 ` Ben Hutchings
@ 2013-12-02 17:39 ` Wyborny, Carolyn
2013-12-02 18:33 ` David Miller
1 sibling, 0 replies; 16+ messages in thread
From: Wyborny, Carolyn @ 2013-12-02 17:39 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, Kirsher, Jeffrey T, netdev, gospo, sassmann
> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Monday, December 02, 2013 9:18 AM
> To: Wyborny, Carolyn
> Cc: David Miller; Kirsher, Jeffrey T; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net 1/8] igb: Update queue reinit function to call dev_close when
> init of queues fails
>
[..]
> My concerns were that after a failure where the interface is effectively down,
> - the interface must be in a consistent state where is it safe to reconfigure the
> interface again or to unbind the driver, and
> - it should be obviously down so the user can then try to bring it up again.
>
> David is saying that you should implement the reconfiguration in such a way that
> you can always roll back to the previous working state in case of a failure (which
> would make my concerns moot). This is definitely a good goal but I'm not
> convinced that it's always possible.
>
Thanks for the clarification Ben, I'll see what I can do to follow Dave's feedback.
Carolyn
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails
2013-12-02 17:18 ` Ben Hutchings
2013-12-02 17:39 ` Wyborny, Carolyn
@ 2013-12-02 18:33 ` David Miller
1 sibling, 0 replies; 16+ messages in thread
From: David Miller @ 2013-12-02 18:33 UTC (permalink / raw)
To: bhutchings; +Cc: carolyn.wyborny, jeffrey.t.kirsher, netdev, gospo, sassmann
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 2 Dec 2013 17:18:16 +0000
> David is saying that you should implement the reconfiguration in such a
> way that you can always roll back to the previous working state in case
> of a failure (which would make my concerns moot). This is definitely a
> good goal but I'm not convinced that it's always possible.
In this case it is always possible.
The only failure possible in these code paths is for a memory
allocation failure. Therefore, without a doubt, trying to allocate
the memory first before making any changes will provably allow perfect
rollback to a working state. In fact, no state will be changed at all
if the allocation fails.
That's the whole point of my suggestion. Do the one thing that
can fail, the memory allocation, before adjusting anything else.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [net 2/8] igb: Fixed Wake On LAN support
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
2013-11-28 7:35 ` [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails Jeff Kirsher
@ 2013-11-28 7:35 ` Jeff Kirsher
2013-11-28 7:35 ` [net 3/8] e1000: prevent oops when adapter is being closed and reset simultaneously Jeff Kirsher
` (5 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:35 UTC (permalink / raw)
To: davem; +Cc: Akeem G Abodunrin, netdev, gospo, sassmann, Jeff Kirsher
From: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
This patch fixes Wake on LAN being reported as supported on some Ethernet
ports, in contrary to Hardware capability.
Signed-off-by: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index b0f3666..c3143da 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2062,14 +2062,15 @@ static void igb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
{
struct igb_adapter *adapter = netdev_priv(netdev);
- wol->supported = WAKE_UCAST | WAKE_MCAST |
- WAKE_BCAST | WAKE_MAGIC |
- WAKE_PHY;
wol->wolopts = 0;
if (!(adapter->flags & IGB_FLAG_WOL_SUPPORTED))
return;
+ wol->supported = WAKE_UCAST | WAKE_MCAST |
+ WAKE_BCAST | WAKE_MAGIC |
+ WAKE_PHY;
+
/* apply any specific unsupported masks here */
switch (adapter->hw.device_id) {
default:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [net 3/8] e1000: prevent oops when adapter is being closed and reset simultaneously
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
2013-11-28 7:35 ` [net 1/8] igb: Update queue reinit function to call dev_close when init of queues fails Jeff Kirsher
2013-11-28 7:35 ` [net 2/8] igb: Fixed Wake On LAN support Jeff Kirsher
@ 2013-11-28 7:35 ` Jeff Kirsher
2013-11-28 7:35 ` [net 4/8] e1000: fix lockdep warning in e1000_reset_task Jeff Kirsher
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:35 UTC (permalink / raw)
To: davem; +Cc: yzhu1, netdev, gospo, sassmann, Jeff Kirsher
From: yzhu1 <yanjun.zhu@windriver.com>
This change is based on a similar change made to e1000e support in
commit bb9e44d0d0f4 ("e1000e: prevent oops when adapter is being closed
and reset simultaneously"). The same issue has also been observed
on the older e1000 cards.
Here, we have increased the RESET_COUNT value to 50 because there are too
many accesses to e1000 nic on stress tests to e1000 nic, it is not enough
to set RESET_COUT 25. Experimentation has shown that it is enough to set
RESET_COUNT 50.
Signed-off-by: yzhu1 <yanjun.zhu@windriver.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000.h | 5 +++++
drivers/net/ethernet/intel/e1000/e1000_main.c | 9 +++++++++
2 files changed, 14 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index 58c1472..e4093d1 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -83,6 +83,11 @@ struct e1000_adapter;
#define E1000_MAX_INTR 10
+/*
+ * Count for polling __E1000_RESET condition every 10-20msec.
+ */
+#define E1000_CHECK_RESET_COUNT 50
+
/* TX/RX descriptor defines */
#define E1000_DEFAULT_TXD 256
#define E1000_MAX_TXD 256
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index e386228..c0f5217 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1440,6 +1440,10 @@ static int e1000_close(struct net_device *netdev)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
+ int count = E1000_CHECK_RESET_COUNT;
+
+ while (test_bit(__E1000_RESETTING, &adapter->flags) && count--)
+ usleep_range(10000, 20000);
WARN_ON(test_bit(__E1000_RESETTING, &adapter->flags));
e1000_down(adapter);
@@ -4963,6 +4967,11 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake)
netif_device_detach(netdev);
if (netif_running(netdev)) {
+ int count = E1000_CHECK_RESET_COUNT;
+
+ while (test_bit(__E1000_RESETTING, &adapter->flags) && count--)
+ usleep_range(10000, 20000);
+
WARN_ON(test_bit(__E1000_RESETTING, &adapter->flags));
e1000_down(adapter);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [net 4/8] e1000: fix lockdep warning in e1000_reset_task
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
` (2 preceding siblings ...)
2013-11-28 7:35 ` [net 3/8] e1000: prevent oops when adapter is being closed and reset simultaneously Jeff Kirsher
@ 2013-11-28 7:35 ` Jeff Kirsher
2013-11-28 7:35 ` [net 5/8] e1000: fix possible reset_task running after adapter down Jeff Kirsher
` (3 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:35 UTC (permalink / raw)
To: davem
Cc: Vladimir Davydov, netdev, gospo, sassmann, Tushar Dave,
Patrick McHardy, Vladimir Davydov, Jeff Kirsher
From: Vladimir Davydov <VDavydov@parallels.com>
The patch fixes the following lockdep warning, which is 100%
reproducible on network restart:
======================================================
[ INFO: possible circular locking dependency detected ]
3.12.0+ #47 Tainted: GF
-------------------------------------------------------
kworker/1:1/27 is trying to acquire lock:
((&(&adapter->watchdog_task)->work)){+.+...}, at: [<ffffffff8108a5b0>] flush_work+0x0/0x70
but task is already holding lock:
(&adapter->mutex){+.+...}, at: [<ffffffffa0177c0a>] e1000_reset_task+0x4a/0xa0 [e1000]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&adapter->mutex){+.+...}:
[<ffffffff810bdb5d>] lock_acquire+0x9d/0x120
[<ffffffff816b8cbc>] mutex_lock_nested+0x4c/0x390
[<ffffffffa017233d>] e1000_watchdog+0x7d/0x5b0 [e1000]
[<ffffffff8108b972>] process_one_work+0x1d2/0x510
[<ffffffff8108ca80>] worker_thread+0x120/0x3a0
[<ffffffff81092c1e>] kthread+0xee/0x110
[<ffffffff816c3d7c>] ret_from_fork+0x7c/0xb0
-> #0 ((&(&adapter->watchdog_task)->work)){+.+...}:
[<ffffffff810bd9c0>] __lock_acquire+0x1710/0x1810
[<ffffffff810bdb5d>] lock_acquire+0x9d/0x120
[<ffffffff8108a5eb>] flush_work+0x3b/0x70
[<ffffffff8108b5d8>] __cancel_work_timer+0x98/0x140
[<ffffffff8108b693>] cancel_delayed_work_sync+0x13/0x20
[<ffffffffa0170cec>] e1000_down_and_stop+0x3c/0x60 [e1000]
[<ffffffffa01775b1>] e1000_down+0x131/0x220 [e1000]
[<ffffffffa0177c12>] e1000_reset_task+0x52/0xa0 [e1000]
[<ffffffff8108b972>] process_one_work+0x1d2/0x510
[<ffffffff8108ca80>] worker_thread+0x120/0x3a0
[<ffffffff81092c1e>] kthread+0xee/0x110
[<ffffffff816c3d7c>] ret_from_fork+0x7c/0xb0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&adapter->mutex);
lock((&(&adapter->watchdog_task)->work));
lock(&adapter->mutex);
lock((&(&adapter->watchdog_task)->work));
*** DEADLOCK ***
3 locks held by kworker/1:1/27:
#0: (events){.+.+.+}, at: [<ffffffff8108b906>] process_one_work+0x166/0x510
#1: ((&adapter->reset_task)){+.+...}, at: [<ffffffff8108b906>] process_one_work+0x166/0x510
#2: (&adapter->mutex){+.+...}, at: [<ffffffffa0177c0a>] e1000_reset_task+0x4a/0xa0 [e1000]
stack backtrace:
CPU: 1 PID: 27 Comm: kworker/1:1 Tainted: GF 3.12.0+ #47
Hardware name: System manufacturer System Product Name/P5B-VM SE, BIOS 0501 05/31/2007
Workqueue: events e1000_reset_task [e1000]
ffffffff820f6000 ffff88007b9dba98 ffffffff816b54a2 0000000000000002
ffffffff820f5e50 ffff88007b9dbae8 ffffffff810ba936 ffff88007b9dbac8
ffff88007b9dbb48 ffff88007b9d8f00 ffff88007b9d8780 ffff88007b9d8f00
Call Trace:
[<ffffffff816b54a2>] dump_stack+0x49/0x5f
[<ffffffff810ba936>] print_circular_bug+0x216/0x310
[<ffffffff810bd9c0>] __lock_acquire+0x1710/0x1810
[<ffffffff8108a5b0>] ? __flush_work+0x250/0x250
[<ffffffff810bdb5d>] lock_acquire+0x9d/0x120
[<ffffffff8108a5b0>] ? __flush_work+0x250/0x250
[<ffffffff8108a5eb>] flush_work+0x3b/0x70
[<ffffffff8108a5b0>] ? __flush_work+0x250/0x250
[<ffffffff8108b5d8>] __cancel_work_timer+0x98/0x140
[<ffffffff8108b693>] cancel_delayed_work_sync+0x13/0x20
[<ffffffffa0170cec>] e1000_down_and_stop+0x3c/0x60 [e1000]
[<ffffffffa01775b1>] e1000_down+0x131/0x220 [e1000]
[<ffffffffa0177c12>] e1000_reset_task+0x52/0xa0 [e1000]
[<ffffffff8108b972>] process_one_work+0x1d2/0x510
[<ffffffff8108b906>] ? process_one_work+0x166/0x510
[<ffffffff8108ca80>] worker_thread+0x120/0x3a0
[<ffffffff8108c960>] ? manage_workers+0x2c0/0x2c0
[<ffffffff81092c1e>] kthread+0xee/0x110
[<ffffffff81092b30>] ? __init_kthread_worker+0x70/0x70
[<ffffffff816c3d7c>] ret_from_fork+0x7c/0xb0
[<ffffffff81092b30>] ? __init_kthread_worker+0x70/0x70
== The issue background ==
The problem occurs, because e1000_down(), which is called under
adapter->mutex by e1000_reset_task(), tries to synchronously cancel
e1000 auxiliary works (reset_task, watchdog_task, phy_info_task,
fifo_stall_task), which take adapter->mutex in their handlers. So the
question is what does adapter->mutex protect there?
The adapter->mutex was introduced by commit 0ef4ee ("e1000: convert to
private mutex from rtnl") as a replacement for rtnl_lock() taken in the
asynchronous handlers. It targeted on fixing a similar lockdep warning
issued when e1000_down() was called under rtnl_lock(), and it fixed it,
but unfortunately it introduced the lockdep warning described above.
Anyway, that said the source of this bug is that the asynchronous works
were made to take rtnl_lock() some time ago, so let's look deeper and
find why it was added there.
The rtnl_lock() was added to asynchronous handlers by commit 338c15
("e1000: fix occasional panic on unload") in order to prevent
asynchronous handlers from execution after the module is unloaded
(e1000_down() is called) as it follows from the comment to the commit:
> Net drivers in general have an issue where timers fired
> by mod_timer or work threads with schedule_work are running
> outside of the rtnl_lock.
>
> With no other lock protection these routines are vulnerable
> to races with driver unload or reset paths.
>
> The longer term solution to this might be a redesign with
> safer locks being taken in the driver to guarantee no
> reentrance, but for now a safe and effective fix is
> to take the rtnl_lock in these routines.
I'm not sure if this locking scheme fixed the problem or just made it
unlikely, although I incline to the latter. Anyway, this was long time
ago when e1000 auxiliary works were implemented as timers scheduling
real work handlers in their routines. The e1000_down() function only
canceled the timers, but left the real handlers running if they were
running, which could result in work execution after module unload.
Today, the e1000 driver uses sane delayed works instead of the pair
timer+work to implement its delayed asynchronous handlers, and the
e1000_down() synchronously cancels all the works so that the problem
that commit 338c15 tried to cope with disappeared, and we don't need any
locks in the handlers any more. Moreover, any locking there can
potentially result in a deadlock.
So, this patch reverts commits 0ef4ee and 338c15.
Fixes: 0ef4eedc2e98 ("e1000: convert to private mutex from rtnl")
Fixes: 338c15e470d8 ("e1000: fix occasional panic on unload")
Cc: Tushar Dave <tushar.n.dave@intel.com>
Cc: Patrick McHardy <kaber@trash.net>
Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000.h | 2 --
drivers/net/ethernet/intel/e1000/e1000_main.c | 36 +++------------------------
2 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index e4093d1..f9313b3 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -317,8 +317,6 @@ struct e1000_adapter {
struct delayed_work watchdog_task;
struct delayed_work fifo_stall_task;
struct delayed_work phy_info_task;
-
- struct mutex mutex;
};
enum e1000_state_t {
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index c0f5217..619b0cb 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -544,21 +544,8 @@ void e1000_down(struct e1000_adapter *adapter)
e1000_clean_all_rx_rings(adapter);
}
-static void e1000_reinit_safe(struct e1000_adapter *adapter)
-{
- while (test_and_set_bit(__E1000_RESETTING, &adapter->flags))
- msleep(1);
- mutex_lock(&adapter->mutex);
- e1000_down(adapter);
- e1000_up(adapter);
- mutex_unlock(&adapter->mutex);
- clear_bit(__E1000_RESETTING, &adapter->flags);
-}
-
void e1000_reinit_locked(struct e1000_adapter *adapter)
{
- /* if rtnl_lock is not held the call path is bogus */
- ASSERT_RTNL();
WARN_ON(in_interrupt());
while (test_and_set_bit(__E1000_RESETTING, &adapter->flags))
msleep(1);
@@ -1316,7 +1303,6 @@ static int e1000_sw_init(struct e1000_adapter *adapter)
e1000_irq_disable(adapter);
spin_lock_init(&adapter->stats_lock);
- mutex_init(&adapter->mutex);
set_bit(__E1000_DOWN, &adapter->flags);
@@ -2329,11 +2315,8 @@ static void e1000_update_phy_info_task(struct work_struct *work)
struct e1000_adapter *adapter = container_of(work,
struct e1000_adapter,
phy_info_task.work);
- if (test_bit(__E1000_DOWN, &adapter->flags))
- return;
- mutex_lock(&adapter->mutex);
+
e1000_phy_get_info(&adapter->hw, &adapter->phy_info);
- mutex_unlock(&adapter->mutex);
}
/**
@@ -2349,9 +2332,6 @@ static void e1000_82547_tx_fifo_stall_task(struct work_struct *work)
struct net_device *netdev = adapter->netdev;
u32 tctl;
- if (test_bit(__E1000_DOWN, &adapter->flags))
- return;
- mutex_lock(&adapter->mutex);
if (atomic_read(&adapter->tx_fifo_stall)) {
if ((er32(TDT) == er32(TDH)) &&
(er32(TDFT) == er32(TDFH)) &&
@@ -2372,7 +2352,6 @@ static void e1000_82547_tx_fifo_stall_task(struct work_struct *work)
schedule_delayed_work(&adapter->fifo_stall_task, 1);
}
}
- mutex_unlock(&adapter->mutex);
}
bool e1000_has_link(struct e1000_adapter *adapter)
@@ -2426,10 +2405,6 @@ static void e1000_watchdog(struct work_struct *work)
struct e1000_tx_ring *txdr = adapter->tx_ring;
u32 link, tctl;
- if (test_bit(__E1000_DOWN, &adapter->flags))
- return;
-
- mutex_lock(&adapter->mutex);
link = e1000_has_link(adapter);
if ((netif_carrier_ok(netdev)) && link)
goto link_up;
@@ -2520,7 +2495,7 @@ link_up:
adapter->tx_timeout_count++;
schedule_work(&adapter->reset_task);
/* exit immediately since reset is imminent */
- goto unlock;
+ return;
}
}
@@ -2548,9 +2523,6 @@ link_up:
/* Reschedule the task */
if (!test_bit(__E1000_DOWN, &adapter->flags))
schedule_delayed_work(&adapter->watchdog_task, 2 * HZ);
-
-unlock:
- mutex_unlock(&adapter->mutex);
}
enum latency_range {
@@ -3499,10 +3471,8 @@ static void e1000_reset_task(struct work_struct *work)
struct e1000_adapter *adapter =
container_of(work, struct e1000_adapter, reset_task);
- if (test_bit(__E1000_DOWN, &adapter->flags))
- return;
e_err(drv, "Reset adapter\n");
- e1000_reinit_safe(adapter);
+ e1000_reinit_locked(adapter);
}
/**
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [net 5/8] e1000: fix possible reset_task running after adapter down
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
` (3 preceding siblings ...)
2013-11-28 7:35 ` [net 4/8] e1000: fix lockdep warning in e1000_reset_task Jeff Kirsher
@ 2013-11-28 7:35 ` Jeff Kirsher
2013-11-28 7:36 ` [net 6/8] ixgbe: ixgbe_fwd_ring_down needs to be static Jeff Kirsher
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:35 UTC (permalink / raw)
To: davem
Cc: Vladimir Davydov, netdev, gospo, sassmann, Tushar Dave,
Patrick McHardy, Vladimir Davydov, Jeff Kirsher
From: Vladimir Davydov <VDavydov@parallels.com>
On e1000_down(), we should ensure every asynchronous work is canceled
before proceeding. Since the watchdog_task can schedule other works
apart from itself, it should be stopped first, but currently it is
stopped after the reset_task. This can result in the following race
leading to the reset_task running after the module unload:
e1000_down_and_stop(): e1000_watchdog():
---------------------- -----------------
cancel_work_sync(reset_task)
schedule_work(reset_task)
cancel_delayed_work_sync(watchdog_task)
The patch moves cancel_delayed_work_sync(watchdog_task) at the beginning
of e1000_down_and_stop() thus ensuring the race is impossible.
Cc: Tushar Dave <tushar.n.dave@intel.com>
Cc: Patrick McHardy <kaber@trash.net>
Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 619b0cb..46e6544 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -494,13 +494,20 @@ static void e1000_down_and_stop(struct e1000_adapter *adapter)
{
set_bit(__E1000_DOWN, &adapter->flags);
- /* Only kill reset task if adapter is not resetting */
- if (!test_bit(__E1000_RESETTING, &adapter->flags))
- cancel_work_sync(&adapter->reset_task);
-
cancel_delayed_work_sync(&adapter->watchdog_task);
+
+ /*
+ * Since the watchdog task can reschedule other tasks, we should cancel
+ * it first, otherwise we can run into the situation when a work is
+ * still running after the adapter has been turned down.
+ */
+
cancel_delayed_work_sync(&adapter->phy_info_task);
cancel_delayed_work_sync(&adapter->fifo_stall_task);
+
+ /* Only kill reset task if adapter is not resetting */
+ if (!test_bit(__E1000_RESETTING, &adapter->flags))
+ cancel_work_sync(&adapter->reset_task);
}
void e1000_down(struct e1000_adapter *adapter)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [net 6/8] ixgbe: ixgbe_fwd_ring_down needs to be static
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
` (4 preceding siblings ...)
2013-11-28 7:35 ` [net 5/8] e1000: fix possible reset_task running after adapter down Jeff Kirsher
@ 2013-11-28 7:36 ` Jeff Kirsher
2013-11-28 7:36 ` [net 7/8] ixgbe: turn NETIF_F_HW_L2FW_DOFFLOAD off by default Jeff Kirsher
2013-11-28 7:36 ` [net 8/8] ixgbe: Make ixgbe_identify_qsfp_module_generic static Jeff Kirsher
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:36 UTC (permalink / raw)
To: davem; +Cc: John Fastabend, netdev, gospo, sassmann, Jeff Kirsher
From: John Fastabend <john.r.fastabend@intel.com>
When compiling with -Wstrict-prototypes gcc catches a static
I missed.
./ixgbe_main.c:4254: warning: no previous prototype for 'ixgbe_fwd_ring_down'
Reported-by: Phillip Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 0c55079..ad9d670 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4251,8 +4251,8 @@ static void ixgbe_disable_fwd_ring(struct ixgbe_fwd_adapter *vadapter,
rx_ring->l2_accel_priv = NULL;
}
-int ixgbe_fwd_ring_down(struct net_device *vdev,
- struct ixgbe_fwd_adapter *accel)
+static int ixgbe_fwd_ring_down(struct net_device *vdev,
+ struct ixgbe_fwd_adapter *accel)
{
struct ixgbe_adapter *adapter = accel->real_adapter;
unsigned int rxbase = accel->rx_base_queue;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [net 7/8] ixgbe: turn NETIF_F_HW_L2FW_DOFFLOAD off by default
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
` (5 preceding siblings ...)
2013-11-28 7:36 ` [net 6/8] ixgbe: ixgbe_fwd_ring_down needs to be static Jeff Kirsher
@ 2013-11-28 7:36 ` Jeff Kirsher
2013-11-28 7:36 ` [net 8/8] ixgbe: Make ixgbe_identify_qsfp_module_generic static Jeff Kirsher
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:36 UTC (permalink / raw)
To: davem; +Cc: John Fastabend, netdev, gospo, sassmann, Jeff Kirsher
From: John Fastabend <john.r.fastabend@intel.com>
NETIF_F_HW_L2FW_DOFFLOAD allows upper layer net devices such
as macvlan to use queues in the hardware to directly submit and
receive skbs.
This creates a subtle change in the datapath though. One change
being the skb may no longer use the root devices qdisc.
Because users may not expect this we can't enable the feature
by default unless the hardware can offload all the software
functionality above it. So for now disable it by default and
let users opt in.
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ad9d670..cc06854 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7986,10 +7986,9 @@ skip_sriov:
NETIF_F_TSO |
NETIF_F_TSO6 |
NETIF_F_RXHASH |
- NETIF_F_RXCSUM |
- NETIF_F_HW_L2FW_DOFFLOAD;
+ NETIF_F_RXCSUM;
- netdev->hw_features = netdev->features;
+ netdev->hw_features = netdev->features | NETIF_F_HW_L2FW_DOFFLOAD;
switch (adapter->hw.mac.type) {
case ixgbe_mac_82599EB:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [net 8/8] ixgbe: Make ixgbe_identify_qsfp_module_generic static
2013-11-28 7:35 [net 0/8][pull request] Intel Wired LAN Driver Updates Jeff Kirsher
` (6 preceding siblings ...)
2013-11-28 7:36 ` [net 7/8] ixgbe: turn NETIF_F_HW_L2FW_DOFFLOAD off by default Jeff Kirsher
@ 2013-11-28 7:36 ` Jeff Kirsher
7 siblings, 0 replies; 16+ messages in thread
From: Jeff Kirsher @ 2013-11-28 7:36 UTC (permalink / raw)
To: davem; +Cc: Mark Rustad, netdev, gospo, sassmann, Jeff Kirsher
From: Mark Rustad <mark.d.rustad@intel.com>
Correct a namespace complaint by making the function static
and moving the prototype into the .c file.
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 3 ++-
drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h | 1 -
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index e4c6760..39217e5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -46,6 +46,7 @@ static bool ixgbe_get_i2c_data(u32 *i2cctl);
static void ixgbe_i2c_bus_clear(struct ixgbe_hw *hw);
static enum ixgbe_phy_type ixgbe_get_phy_type_from_id(u32 phy_id);
static s32 ixgbe_get_phy_id(struct ixgbe_hw *hw);
+static s32 ixgbe_identify_qsfp_module_generic(struct ixgbe_hw *hw);
/**
* ixgbe_identify_phy_generic - Get physical layer module
@@ -1164,7 +1165,7 @@ err_read_i2c_eeprom:
*
* Searches for and identifies the QSFP module and assigns appropriate PHY type
**/
-s32 ixgbe_identify_qsfp_module_generic(struct ixgbe_hw *hw)
+static s32 ixgbe_identify_qsfp_module_generic(struct ixgbe_hw *hw)
{
struct ixgbe_adapter *adapter = hw->back;
s32 status = IXGBE_ERR_PHY_ADDR_INVALID;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
index aae900a..fffcbdd 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
@@ -145,7 +145,6 @@ s32 ixgbe_get_phy_firmware_version_generic(struct ixgbe_hw *hw,
s32 ixgbe_reset_phy_nl(struct ixgbe_hw *hw);
s32 ixgbe_identify_module_generic(struct ixgbe_hw *hw);
s32 ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw);
-s32 ixgbe_identify_qsfp_module_generic(struct ixgbe_hw *hw);
s32 ixgbe_get_sfp_init_sequence_offsets(struct ixgbe_hw *hw,
u16 *list_offset,
u16 *data_offset);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread