Re: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
Sreenivasa Honnur wrote: - Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails in s2io_add_isr. - Added two utility functions remove_msix_isr and remove_inta_isr to eliminate code duplication. - Incorporated following review comments from Jeff - Removed redundant stats-mem_freed and synchronize_irq call - do_rem_msix_isr is renamed as remove_msix_isr - do_rem_inta_isr is renamed as remove_inta_isr - (Resubmit third time) ditto, does not apply - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
Jeff, We got an ACK back last week from Dave Miller that the following 2 patches were already applied - reply follows. [Ram] The version numbers are different for the 2 patches. Was the MSI-X leak bug fix applied prior to resubmission (we had swapped the version numbers on the patches for resubmission). [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails +#define DRV_VERSION 2.0.26.6 [PATCH 2.6.24 1/1]S2io: Support for add/delete/store/restore ethernet addresses +#define DRV_VERSION 2.0.26.7 I applied only the leak fix to the stable branch, and only the ethernet address support patch to the 2.6.25 development tree. The bug fix will show up later when the 2.6.25 development tree gets rebased to upstream (which will have the leak fix by then). This is how I do things, and that's why changing the DRV_VERSION to 2.0.26.7 made no sense. Therefore it makes the most sense to bump driver version numbers only when things are entirely self contained. Ram -Original Message- From: Jeff Garzik [mailto:[EMAIL PROTECTED] Sent: Friday, November 23, 2007 7:03 PM To: Sreenivasa Honnur Cc: netdev@vger.kernel.org; support Subject: Re: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails Sreenivasa Honnur wrote: - Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails in s2io_add_isr. - Added two utility functions remove_msix_isr and remove_inta_isr to eliminate code duplication. - Incorporated following review comments from Jeff - Removed redundant stats-mem_freed and synchronize_irq call - do_rem_msix_isr is renamed as remove_msix_isr - do_rem_inta_isr is renamed as remove_inta_isr - (Resubmit third time) ditto, does not apply - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
Ramkrishna Vepa wrote: Jeff, We got an ACK back last week from Dave Miller that the following 2 patches were already applied - reply follows. whoops sorry, I see that now. yep, they are applied. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
- Fixed memory leak by freeing MSI-X local entry memories when vector allocation
fails in s2io_add_isr.
- Added two utility functions remove_msix_isr and remove_inta_isr to eliminate
code duplication.
- Incorporated following review comments from Jeff
- Removed redundant stats-mem_freed and synchronize_irq call
- do_rem_msix_isr is renamed as remove_msix_isr
- do_rem_inta_isr is renamed as remove_inta_isr
- (Resubmit third time)
Signed-off-by: Sreenivasa Honnur [EMAIL PROTECTED]
---
diff -Nurp org/drivers/net/s2io.c patch1/drivers/net/s2io.c
--- org/drivers/net/s2io.c 2007-11-12 22:00:23.0 +0530
+++ patch1/drivers/net/s2io.c 2007-11-12 23:53:19.0 +0530
@@ -84,7 +84,7 @@
#include s2io.h
#include s2io-regs.h
-#define DRV_VERSION 2.0.26.5
+#define DRV_VERSION 2.0.26.6
/* S2io Driver name version. */
static char s2io_driver_name[] = Neterion;
@@ -3775,6 +3775,40 @@ static int __devinit s2io_test_msi(struc
return err;
}
+
+static void remove_msix_isr(struct s2io_nic *sp)
+{
+ int i;
+ u16 msi_control;
+
+ for (i = 0; i MAX_REQUESTED_MSI_X; i++) {
+ if (sp-s2io_entries[i].in_use ==
+ MSIX_REGISTERED_SUCCESS) {
+ int vector = sp-entries[i].vector;
+ void *arg = sp-s2io_entries[i].arg;
+ free_irq(vector, arg);
+ }
+ }
+
+ kfree(sp-entries);
+ kfree(sp-s2io_entries);
+ sp-entries = NULL;
+ sp-s2io_entries = NULL;
+
+ pci_read_config_word(sp-pdev, 0x42, msi_control);
+ msi_control = 0xFFFE; /* Disable MSI */
+ pci_write_config_word(sp-pdev, 0x42, msi_control);
+
+ pci_disable_msix(sp-pdev);
+}
+
+static void remove_inta_isr(struct s2io_nic *sp)
+{
+ struct net_device *dev = sp-dev;
+
+ free_irq(sp-pdev-irq, dev);
+}
+
/* * *
* Functions defined below concern the OS part of the driver *
* * */
@@ -3809,28 +3843,9 @@ static int s2io_open(struct net_device *
int ret = s2io_enable_msi_x(sp);
if (!ret) {
- u16 msi_control;
-
ret = s2io_test_msi(sp);
-
/* rollback MSI-X, will re-enable during add_isr() */
- kfree(sp-entries);
- sp-mac_control.stats_info-sw_stat.mem_freed +=
- (MAX_REQUESTED_MSI_X *
- sizeof(struct msix_entry));
- kfree(sp-s2io_entries);
- sp-mac_control.stats_info-sw_stat.mem_freed +=
- (MAX_REQUESTED_MSI_X *
- sizeof(struct s2io_msix_entry));
- sp-entries = NULL;
- sp-s2io_entries = NULL;
-
- pci_read_config_word(sp-pdev, 0x42, msi_control);
- msi_control = 0xFFFE; /* Disable MSI */
- pci_write_config_word(sp-pdev, 0x42, msi_control);
-
- pci_disable_msix(sp-pdev);
-
+ remove_msix_isr(sp);
}
if (ret) {
@@ -6719,15 +6734,22 @@ static int s2io_add_isr(struct s2io_nic
}
}
if (err) {
+ remove_msix_isr(sp);
DBG_PRINT(ERR_DBG,%s:MSI-X-%d registration
failed\n, dev-name, i);
- DBG_PRINT(ERR_DBG, Returned: %d\n, err);
- return -1;
+ DBG_PRINT(ERR_DBG, %s: defaulting to INTA\n,
+dev-name);
+ sp-config.intr_type = INTA;
+ break;
}
sp-s2io_entries[i].in_use = MSIX_REGISTERED_SUCCESS;
}
- printk(MSI-X-TX %d entries enabled\n,msix_tx_cnt);
- printk(MSI-X-RX %d entries enabled\n,msix_rx_cnt);
+ if (!err) {
+ printk(KERN_INFO MSI-X-TX %d entries enabled\n,
+ msix_tx_cnt);
+ printk(KERN_INFO MSI-X-RX %d entries enabled\n,
+ msix_rx_cnt);
+ }
}
if (sp-config.intr_type == INTA) {
err = request_irq((int) sp-pdev-irq, s2io_isr, IRQF_SHARED,
@@ -6742,40 +6764,10 @@ static int s2io_add_isr(struct s2io_nic
}
static void s2io_rem_isr(struct s2io_nic * sp)
{
- struct net_device *dev = sp-dev;
- struct swStat *stats = sp-mac_control.stats_info-sw_stat;
-
- if (sp-config.intr_type == MSI_X) {
-
Re: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
From: Sreenivasa Honnur [EMAIL PROTECTED] Date: Wed, 14 Nov 2007 04:29:07 -0500 (EST) - Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails in s2io_add_isr. - Added two utility functions remove_msix_isr and remove_inta_isr to eliminate code duplication. - Incorporated following review comments from Jeff - Removed redundant stats-mem_freed and synchronize_irq call - do_rem_msix_isr is renamed as remove_msix_isr - do_rem_inta_isr is renamed as remove_inta_isr - (Resubmit third time) Signed-off-by: Sreenivasa Honnur [EMAIL PROTECTED] Applied to net-2.6, thanks! - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
Sreenivasa Honnur wrote: - Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails in s2io_add_isr. - Added two utility functions remove_msix_isr and remove_inta_isr to eliminate code duplication. - Implemented following review comments from Jeff - Removed redundant stats-mem_freed and synchronize_irq call - do_rem_msix_isr is renamed as remove_msix_isr - do_rem_inta_isr is renamed as remove_inta_isr Signed-off-by: Sreenivasa Honnur [EMAIL PROTECTED] Signed-off-by: Ramkrishna Vepa [EMAIL PROTECTED] this patch should go into 2.6.24-rc, but it doesn't apply to upstream. This may be because it requires S2io: Support for add/delete/store/restore ethernet addresses, I am guessing? We want to reverse the order of those two patches, because S2io: Support for add/delete/store/restore ethernet addresses is more appropriate for non-bug-fix 2.6.24 rather than current bugfix-only 2.6.24-rc. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
- Fixed memory leak by freeing MSI-X local entry memories when vector allocation
fails in s2io_add_isr.
- Added two utility functions remove_msix_isr and remove_inta_isr to eliminate
code duplication.
- Implemented following review comments from Jeff
- Removed redundant stats-mem_freed and synchronize_irq call
- do_rem_msix_isr is renamed as remove_msix_isr
- do_rem_inta_isr is renamed as remove_inta_isr
Signed-off-by: Sreenivasa Honnur [EMAIL PROTECTED]
Signed-off-by: Ramkrishna Vepa [EMAIL PROTECTED]
---
diff -Nurp org/drivers/net/s2io.c patch1/drivers/net/s2io.c
--- org/drivers/net/s2io.c 2007-10-30 23:31:09.0 +0530
+++ patch1/drivers/net/s2io.c 2007-10-31 04:12:00.0 +0530
@@ -84,7 +84,7 @@
#include s2io.h
#include s2io-regs.h
-#define DRV_VERSION 2.0.26.6
+#define DRV_VERSION 2.0.26.7
/* S2io Driver name version. */
static char s2io_driver_name[] = Neterion;
@@ -3775,6 +3775,40 @@ static int __devinit s2io_test_msi(struc
return err;
}
+
+static void remove_msix_isr(struct s2io_nic *sp)
+{
+ int i;
+ u16 msi_control;
+
+ for (i = 0; i MAX_REQUESTED_MSI_X; i++) {
+ if (sp-s2io_entries[i].in_use ==
+ MSIX_REGISTERED_SUCCESS) {
+ int vector = sp-entries[i].vector;
+ void *arg = sp-s2io_entries[i].arg;
+ free_irq(vector, arg);
+ }
+ }
+
+ kfree(sp-entries);
+ kfree(sp-s2io_entries);
+ sp-entries = NULL;
+ sp-s2io_entries = NULL;
+
+ pci_read_config_word(sp-pdev, 0x42, msi_control);
+ msi_control = 0xFFFE; /* Disable MSI */
+ pci_write_config_word(sp-pdev, 0x42, msi_control);
+
+ pci_disable_msix(sp-pdev);
+}
+
+static void remove_inta_isr(struct s2io_nic *sp)
+{
+ struct net_device *dev = sp-dev;
+
+ free_irq(sp-pdev-irq, dev);
+}
+
/* * *
* Functions defined below concern the OS part of the driver *
* * */
@@ -3809,28 +3843,9 @@ static int s2io_open(struct net_device *
int ret = s2io_enable_msi_x(sp);
if (!ret) {
- u16 msi_control;
-
ret = s2io_test_msi(sp);
-
/* rollback MSI-X, will re-enable during add_isr() */
- kfree(sp-entries);
- sp-mac_control.stats_info-sw_stat.mem_freed +=
- (MAX_REQUESTED_MSI_X *
- sizeof(struct msix_entry));
- kfree(sp-s2io_entries);
- sp-mac_control.stats_info-sw_stat.mem_freed +=
- (MAX_REQUESTED_MSI_X *
- sizeof(struct s2io_msix_entry));
- sp-entries = NULL;
- sp-s2io_entries = NULL;
-
- pci_read_config_word(sp-pdev, 0x42, msi_control);
- msi_control = 0xFFFE; /* Disable MSI */
- pci_write_config_word(sp-pdev, 0x42, msi_control);
-
- pci_disable_msix(sp-pdev);
-
+ remove_msix_isr(sp);
}
if (ret) {
@@ -6864,15 +6879,23 @@ static int s2io_add_isr(struct s2io_nic
}
}
if (err) {
+ remove_msix_isr(sp);
+
DBG_PRINT(ERR_DBG,%s:MSI-X-%d registration
failed\n, dev-name, i);
- DBG_PRINT(ERR_DBG, Returned: %d\n, err);
- return -1;
+ DBG_PRINT(ERR_DBG, %s: defaulting to INTA\n,
+ dev-name);
+ sp-config.intr_type = INTA;
+ break;
}
sp-s2io_entries[i].in_use = MSIX_REGISTERED_SUCCESS;
}
- printk(MSI-X-TX %d entries enabled\n,msix_tx_cnt);
- printk(MSI-X-RX %d entries enabled\n,msix_rx_cnt);
+ if (!err) {
+ DBG_PRINT(INFO_DBG, MSI-X-TX %d entries enabled\n,
+ msix_tx_cnt);
+ DBG_PRINT(INFO_DBG, MSI-X-RX %d entries enabled\n,
+ msix_rx_cnt);
+ }
}
if (sp-config.intr_type == INTA) {
err = request_irq((int) sp-pdev-irq, s2io_isr, IRQF_SHARED,
@@ -6887,40 +6910,10 @@ static int s2io_add_isr(struct s2io_nic
}
static void s2io_rem_isr(struct s2io_nic * sp)
{
- struct net_device *dev = sp-dev;
- struct swStat *stats = sp-mac_control.stats_info-sw_stat;
-
- if (sp-config.intr_type ==
Re: [PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
Sivakumar Subramani wrote: - Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails in s2io_add_isr. - Added two utility functions do_rem_msix_isr and do_rem_inta_isr to eliminate code duplication. Signed-off-by: Veena Parat [EMAIL PROTECTED] Signed-off-by: Ramkrishna Vepa [EMAIL PROTECTED] Signed-off-by: Santosh Rastapur [EMAIL PROTECTED] Comments: 1) stats-mem_freed is redundant to general kernel debugging facilities 2) synchronize_irq() is redundant, free_irq() does same 3) rem isn't very clear. few if any other kernel drivers use this to indicate remove 4) do_ prefix is redundant 5) scripts/checkpatch.pl fails on this patch. please clean up problems. The main issue for this patch is #2, though presumably the other stuff can be cleaned on the next resubmit - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2.6.24 1/1]S2io: Fixed memory leak by freeing MSI-X local entry memories when vector allocation fails
- Fixed memory leak by freeing MSI-X local entry memories when vector allocation
fails in s2io_add_isr.
- Added two utility functions do_rem_msix_isr and do_rem_inta_isr to eliminate
code duplication.
Signed-off-by: Veena Parat [EMAIL PROTECTED]
Signed-off-by: Ramkrishna Vepa [EMAIL PROTECTED]
Signed-off-by: Santosh Rastapur [EMAIL PROTECTED]
---
diff -Nurp 2-0-26-6/drivers/net/s2io.c 2-0-26-7/drivers/net/s2io.c
--- 2-0-26-6/drivers/net/s2io.c 2007-10-24 09:20:39.0 -0700
+++ 2-0-26-7/drivers/net/s2io.c 2007-10-24 09:46:16.0 -0700
@@ -84,7 +84,7 @@
#include s2io.h
#include s2io-regs.h
-#define DRV_VERSION 2.0.26.6
+#define DRV_VERSION 2.0.26.7
/* S2io Driver name version. */
static char s2io_driver_name[] = Neterion;
@@ -3775,6 +3775,46 @@ static int __devinit s2io_test_msi(struc
return err;
}
+
+static void do_rem_msix_isr(struct s2io_nic * sp)
+{
+ struct swStat *stats = sp-mac_control.stats_info-sw_stat;
+ int i;
+ u16 msi_control;
+
+ for (i=1; (sp-s2io_entries[i].in_use ==
+ MSIX_REGISTERED_SUCCESS); i++) {
+ int vector = sp-entries[i].vector;
+ void *arg = sp-s2io_entries[i].arg;
+
+ synchronize_irq(vector);
+ free_irq(vector, arg);
+ }
+
+ kfree(sp-entries);
+ stats-mem_freed +=
+ (MAX_REQUESTED_MSI_X * sizeof(struct msix_entry));
+ kfree(sp-s2io_entries);
+ stats-mem_freed +=
+ (MAX_REQUESTED_MSI_X * sizeof(struct s2io_msix_entry));
+ sp-entries = NULL;
+ sp-s2io_entries = NULL;
+
+ pci_read_config_word(sp-pdev, 0x42, msi_control);
+ msi_control = 0xFFFE; /* Disable MSI */
+ pci_write_config_word(sp-pdev, 0x42, msi_control);
+
+ pci_disable_msix(sp-pdev);
+}
+
+static void do_rem_inta_isr(struct s2io_nic * sp)
+{
+ struct net_device *dev = sp-dev;
+
+ synchronize_irq(sp-pdev-irq);
+ free_irq(sp-pdev-irq, dev);
+}
+
/* * *
* Functions defined below concern the OS part of the driver *
* * */
@@ -3809,28 +3849,11 @@ static int s2io_open(struct net_device *
int ret = s2io_enable_msi_x(sp);
if (!ret) {
- u16 msi_control;
ret = s2io_test_msi(sp);
/* rollback MSI-X, will re-enable during add_isr() */
- kfree(sp-entries);
- sp-mac_control.stats_info-sw_stat.mem_freed +=
- (MAX_REQUESTED_MSI_X *
- sizeof(struct msix_entry));
- kfree(sp-s2io_entries);
- sp-mac_control.stats_info-sw_stat.mem_freed +=
- (MAX_REQUESTED_MSI_X *
- sizeof(struct s2io_msix_entry));
- sp-entries = NULL;
- sp-s2io_entries = NULL;
-
- pci_read_config_word(sp-pdev, 0x42, msi_control);
- msi_control = 0xFFFE; /* Disable MSI */
- pci_write_config_word(sp-pdev, 0x42, msi_control);
-
- pci_disable_msix(sp-pdev);
-
+ do_rem_msix_isr(sp);
}
if (ret) {
@@ -6864,15 +6887,22 @@ static int s2io_add_isr(struct s2io_nic
}
}
if (err) {
+ do_rem_msix_isr(sp);
+
DBG_PRINT(ERR_DBG,%s:MSI-X-%d registration
failed\n, dev-name, i);
- DBG_PRINT(ERR_DBG, Returned: %d\n, err);
- return -1;
+
+ DBG_PRINT(ERR_DBG, %s: Defaulting to INTA\n,
+ dev-name);
+ sp-config.intr_type = INTA;
+ break;
}
sp-s2io_entries[i].in_use = MSIX_REGISTERED_SUCCESS;
}
- printk(MSI-X-TX %d entries enabled\n,msix_tx_cnt);
- printk(MSI-X-RX %d entries enabled\n,msix_rx_cnt);
+ if(!err) {
+ printk(MSI-X-TX %d entries enabled\n,msix_tx_cnt);
+ printk(MSI-X-RX %d entries enabled\n,msix_rx_cnt);
+ }
}
if (sp-config.intr_type == INTA) {
err = request_irq((int) sp-pdev-irq, s2io_isr, IRQF_SHARED,
@@ -6887,40 +6917,10 @@ static int s2io_add_isr(struct s2io_nic
}
static void s2io_rem_isr(struct s2io_nic * sp)
{
- struct net_device *dev = sp-dev;
- struct swStat *stats = sp-mac_control.stats_info-sw_stat;
-
- if (sp-config.intr_type == MSI_X) {
- int
