diff mbox

[Intel-wired-lan] igb: use igb_adapter->io_addr instead of e1000_hw->hw_addr

Message ID alpine.NEB.2.20.17.1611100544220.6177@chris.i8u.org
State Awaiting Upstream, archived
Delegated to: David Miller
Headers show

Commit Message

Hisashi T Fujinaka Nov. 10, 2016, 1:48 p.m. UTC 
On Thu, 10 Nov 2016, Corinna Vinschen wrote:

> On Nov  8 11:33, Alexander Duyck wrote:
...
>> The question I would have is what is reading the device when it is in
>> this state.  The watchdog and any other functions that would read the
>> device should be disabled.
>>
>> One possibility could be a race between a call to igb_close and the
>> igb_suspend function.  We have seen some of those pop up recently on
>> ixgbe and it looks like igb has the same bug.  We should probably be
>> using the rtnl_lock to guarantee that netif_device_detach and the call
>> to __igb_close are completed before igb_close could possibly be called
>> by the network stack.
>
> Do you have a pointer to the related ixgbe patch, by any chance?
...
>> The thing is that a suspended device should not be accessed at all.
>> If we are accessing it while it is suspended then that is a bug.  If
>> you could throw a WARN_ON call in igb_rd32 to capture where this is
>> being triggered that might be useful.
>>
>>> - Otherwise assume it's actually a surprise removal.  In theory that
>>>   should somehow trigger a device removal sequence, kind of like
>>>   calling igb_remove, no?
>>
>> Well a read of the MMIO region while suspended is more of a surprise
>> read since there shouldn't be anything going on.  We need to isolate
>> where that read is coming from and fix it.
>
> That would be ideal, but the problem couldn't be reproduced yet apart
> from at a customer's customer site.  It's not clear yet if we can access
> the machine for further testing.

Here's the initial patch for igb I have, but it's on hold awaiting more
changes in ixgbe regarding AER.

Comments

Corinna Vinschen Nov. 10, 2016, 5:28 p.m. UTC | #1 
On Nov 10 05:48, Hisashi T Fujinaka wrote:
> On Thu, 10 Nov 2016, Corinna Vinschen wrote:
> > On Nov  8 11:33, Alexander Duyck wrote:
> ...
> > > The question I would have is what is reading the device when it is in
> > > this state.  The watchdog and any other functions that would read the
> > > device should be disabled.
> > > 
> > > One possibility could be a race between a call to igb_close and the
> > > igb_suspend function.  We have seen some of those pop up recently on
> > > ixgbe and it looks like igb has the same bug.  We should probably be
> > > using the rtnl_lock to guarantee that netif_device_detach and the call
> > > to __igb_close are completed before igb_close could possibly be called
> > > by the network stack.
> > 
> > Do you have a pointer to the related ixgbe patch, by any chance?
> ...
> Here's the initial patch for igb I have, but it's on hold awaiting more
> changes in ixgbe regarding AER.

Thanks a lot!


Corinna
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 4feca69..ce5add6 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3275,7 +3275,10 @@  static int __igb_close(struct net_device *netdev, bool suspending)
 
 int igb_close(struct net_device *netdev)
 {
-	return __igb_close(netdev, false);
+	if (netif_device_present(netdev))
+		return __igb_close(netdev, false);
+
+	return 0;
 }
 
 /**
@@ -7537,6 +7540,7 @@  static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	int retval = 0;
 #endif
 
+	rtnl_lock();
 	netif_device_detach(netdev);
 
 	if (netif_running(netdev))
@@ -7545,6 +7549,7 @@  static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	igb_ptp_suspend(adapter);
 
 	igb_clear_interrupt_scheme(adapter);
+	rtnl_unlock();
 
 #ifdef CONFIG_PM
 	retval = pci_save_state(pdev);
@@ -7663,16 +7668,17 @@  static int igb_resume(struct device *dev)
 
 	wr32(E1000_WUS, ~0);
 
-	if (netdev->flags & IFF_UP) {
-		rtnl_lock();
+	rtnl_lock();
+
+	if (!err && netif_running(netdev))
 		err = __igb_open(netdev, true);
-		rtnl_unlock();
-		if (err)
-			return err;
-	}
 
-	netif_device_attach(netdev);
-	return 0;
+	if (!err)
+		netif_device_attach(netdev);
+
+	rtnl_unlock();
+
+	return err;
 }
 
 static int igb_runtime_idle(struct device *dev)