Hi,
We have a problem where ixgb's EEPROM update is blocking e.g. routing
updates for too long in kernels starting from 2.6.21.
I thought that this could be solved by dropping RTNL, but I have so
far failed to fully convince myself that this is safe even if I use
some synchronization method for concurrent invocations of the
ethtool's EEPROM routines.
My question for you is, do you think that dropping the RTNL as, for
example, in the attached patch is safe? The used mutex should protect
the EEPROM from concurrent invocations that are coming via ethtool's
interface, but is there some other places that should also acquire this
mutex before proceeding?
--
Best Regards
Hemmo Nieminen
diff -rubN linux-2.6.21-standard.orig/drivers/net/ixgb/ixgb_ethtool.c
linux-2.6.21-standard.modded/drivers/net/ixgb/ixgb_ethtool.c
--- linux-2.6.21-standard.orig/drivers/net/ixgb/ixgb_ethtool.c 2013-05-29
14:47:03.285590569 +0300
+++ linux-2.6.21-standard.modded/drivers/net/ixgb/ixgb_ethtool.c
2013-05-30 19:41:49.809462787 +0300
@@ -432,6 +432,10 @@
goto geeprom_error;
}
+ dev_hold(dev);
+ rtnl_unlock();
+ mutex_lock(&hw->eeprom_lock);
+
eeprom->magic = hw->vendor_id | (hw->device_id << 16);
max_len = ixgb_get_eeprom_len(netdev);
@@ -449,8 +453,10 @@
eeprom_buff = kmalloc(sizeof(uint16_t) *
(last_word - first_word + 1), GFP_KERNEL);
- if(!eeprom_buff)
- return -ENOMEM;
+ if(!eeprom_buff) {
+ retval = -ENOMEM;
+ goto geeprom_error;
+ }
/* note the eeprom was good because the driver loaded */
for(i = 0; i <= (last_word - first_word); i++) {
@@ -462,6 +468,9 @@
kfree(eeprom_buff);
geeprom_error:
+ mutex_unlock(&hw->eeprom_lock);
+ rtnl_lock();
+ dev_put(dev);
return ret_val;
}
@@ -479,13 +488,21 @@
if(eeprom->len == 0)
return -EINVAL;
- if(eeprom->magic != (hw->vendor_id | (hw->device_id << 16)))
- return -EFAULT;
+ dev_hold(dev);
+ rtnl_unlock();
+ mutex_lock(&hw->eeprom_lock);
+
+ if(eeprom->magic != (hw->vendor_id | (hw->device_id << 16))) {
+ retval = -EFAULT;
+ goto out;
+ }
max_len = ixgb_get_eeprom_len(netdev);
- if(eeprom->offset > eeprom->offset + eeprom->len)
- return -EINVAL;
+ if(eeprom->offset > eeprom->offset + eeprom->len) {
+ retval = -EINVAL;
+ goto out;
+ }
if((eeprom->offset + eeprom->len) > max_len)
eeprom->len = (max_len - eeprom->offset);
@@ -493,8 +510,10 @@
first_word = eeprom->offset >> 1;
last_word = (eeprom->offset + eeprom->len - 1) >> 1;
eeprom_buff = kmalloc(max_len, GFP_KERNEL);
- if(!eeprom_buff)
- return -ENOMEM;
+ if(!eeprom_buff) {
+ retval = -ENOMEM;
+ goto out;
+ }
ptr = (void *)eeprom_buff;
@@ -520,6 +539,10 @@
ixgb_update_eeprom_checksum(hw);
kfree(eeprom_buff);
+out:
+ mutex_unlock(&hw->eeprom_lock);
+ rtnl_lock();
+ dev_put(dev);
return 0;
}
@@ -535,7 +558,9 @@
strncpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);
drvinfo->n_stats = IXGB_STATS_LEN;
drvinfo->regdump_len = ixgb_get_regs_len(netdev);
+ mutex_lock(&adapter->hw->eeprom_lock);
drvinfo->eedump_len = ixgb_get_eeprom_len(netdev);
+ mutex_unlock(&adapter->hw->eeprom_lock);
}
static void
diff -rubN linux-2.6.21-standard.orig/drivers/net/ixgb/ixgb_hw.h
linux-2.6.21-standard.modded/drivers/net/ixgb/ixgb_hw.h
--- linux-2.6.21-standard.orig/drivers/net/ixgb/ixgb_hw.h 2013-05-29
14:47:03.285590569 +0300
+++ linux-2.6.21-standard.modded/drivers/net/ixgb/ixgb_hw.h 2013-05-30
19:38:32.663046273 +0300
@@ -29,6 +29,7 @@
#ifndef _IXGB_HW_H_
#define _IXGB_HW_H_
+#include <linux/mutex.h>
#include "ixgb_osdep.h"
/* Enums */
@@ -715,6 +716,7 @@
unsigned long io_base; /* Our I/O mapped location */
uint32_t lastLFC;
uint32_t lastRFC;
+ mutex eeprom_lock;
};
/* Statistics reported by the hardware */
diff -rubN linux-2.6.21-standard.orig/drivers/net/ixgb/ixgb_main.c
linux-2.6.21-standard.modded/drivers/net/ixgb/ixgb_main.c
--- linux-2.6.21-standard.orig/drivers/net/ixgb/ixgb_main.c 2013-05-29
14:47:03.281590519 +0300
+++ linux-2.6.21-standard.modded/drivers/net/ixgb/ixgb_main.c 2013-05-30
19:45:01.635814468 +0300
@@ -401,6 +401,8 @@
mmio_start = pci_resource_start(pdev, BAR_0);
mmio_len = pci_resource_len(pdev, BAR_0);
+ mutex_init(&adapter->hw->eeprom_lock);
+
adapter->hw.hw_addr = ioremap(mmio_start, mmio_len);
if(!adapter->hw.hw_addr) {
err = -EIO;
------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite
It's a free troubleshooting tool designed for production
Get down to code-level detail for bottlenecks, with <2% overhead.
Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap2
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit
http://communities.intel.com/community/wired
