Skip to contentSkip to conversation

NET: Change pci_enable_device to pci_reenable_device to keep device enable balance

RFC PATCH In review Brandon Philips 5 participants 5 messages Aug 2, 2007 – Aug 9, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Brandon Philips

author
#0 lore raw

RFC PATCH 1/5 NET: Change pci_enable_device to pci_reenable_device to keep device enable balance

On a slot_reset event pci_disable_device() is never called so calling pci_enable_device() will unbalance the enable count.

Signed-off-by Brandon Philips <[email protected]>
---
 drivers/net/e100.c             |    2 +-
 drivers/net/e1000/e1000_main.c |    2 +-
 drivers/net/ixgb/ixgb_main.c   |    2 +-
 drivers/net/s2io.c             |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
drivers/net/e100.c +1 -1
Index: linux-2.6/drivers/net/e100.c
===================================================================
@@ -2828,7 +2828,7 @@ static pci_ers_result_t e100_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct nic *nic = netdev_priv(netdev);
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "e100: Cannot re-enable PCI device after reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;
 	}
drivers/net/e1000/e1000_main.c +1 -1
Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================
@@ -5270,7 +5270,7 @@ static pci_ers_result_t e1000_io_slot_re
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct e1000_adapter *adapter = netdev->priv;
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "e1000: Cannot re-enable PCI device after reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;
 	}
drivers/net/ixgb/ixgb_main.c +1 -1
Index: linux-2.6/drivers/net/ixgb/ixgb_main.c
===================================================================
@@ -2294,7 +2294,7 @@ static pci_ers_result_t ixgb_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
 
-	if(pci_enable_device(pdev)) {
+	if(pci_reenable_device(pdev)) {
 		DPRINTK(PROBE, ERR, "Cannot re-enable PCI device after reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;
 	}
drivers/net/s2io.c +1 -1
Index: linux-2.6/drivers/net/s2io.c
===================================================================
@@ -7833,7 +7833,7 @@ static pci_ers_result_t s2io_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct s2io_nic *sp = netdev->priv;
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "s2io: "
 		       "Cannot re-enable PCI device after reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;
#2 lore raw

RE: [patch 1/5][RFC] NET: Change pci_enable_device topci_reenable_device to keep device enable balance

In reply to Brandon Philips

Brandon,

Before slot_reset event is called io_error_detected could be called (where pci_disable_device() is called), right?

The pci_reenable_device() will call enable only if the device was enabled before and would not be enabled if the device were disabled. Is this the intended behavior?

Ram

Quoted 2 lines

-----Original Message-----
From: [email protected]

[mailto:[email protected]]

Quoted 38 lines

On Behalf Of Brandon Philips
Sent: Thursday, August 02, 2007 3:44 PM
To: [email protected]
Cc: [email protected]; Brandon Philips
Subject: [patch 1/5][RFC] NET: Change pci_enable_device topci_reenable_device to keep device enable balance

On a slot_reset event pci_disable_device() is never called so calling pci_enable_device() will unbalance the enable count.

Signed-off-by Brandon Philips <[email protected]>
---
 drivers/net/e100.c             |    2 +-
 drivers/net/e1000/e1000_main.c |    2 +-
 drivers/net/ixgb/ixgb_main.c   |    2 +-
 drivers/net/s2io.c             |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/net/e100.c
===================================================================

drivers/net/e100.c +1 -1
@@ -2828,7 +2828,7 @@ static pci_ers_result_t e100_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct nic *nic = netdev_priv(netdev);
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "e100: Cannot re-enable PCI device after
 reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;

}
Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================

drivers/net/e1000/e1000_main.c static pci_ers_result_t e1000_io_slot_re · line 5274

Quoted 6 lines
drivers/net/e1000/e1000_main.c +1 -1
@@ -5270,7 +5270,7 @@ static pci_ers_result_t e1000_io_slot_re
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct e1000_adapter *adapter = netdev->priv;
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "e1000: Cannot re-enable PCI device

after

Quoted 36 lines
drivers/net/e1000/e1000_main.c +0 -0
 reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;

}
Index: linux-2.6/drivers/net/ixgb/ixgb_main.c
===================================================================

drivers/net/ixgb/ixgb_main.c +1 -1
@@ -2294,7 +2294,7 @@ static pci_ers_result_t ixgb_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
 
-	if(pci_enable_device(pdev)) {
+	if(pci_reenable_device(pdev)) {
 		DPRINTK(PROBE, ERR, "Cannot re-enable PCI device after
 reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;

}
Index: linux-2.6/drivers/net/s2io.c
===================================================================

drivers/net/s2io.c +1 -3
@@ -7833,7 +7833,7 @@ static pci_ers_result_t s2io_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct s2io_nic *sp = netdev->priv;
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "s2io: "
 		       "Cannot re-enable PCI device after reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;
 
--
-

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

#3 lore raw

Re: [patch 1/5][RFC] NET: Change pci_enable_device topci_reenable_device to keep device enable balance

Quoted 2 lines · Ramkrishna Vepa

Before slot_reset event is called io_error_detected could be called (where pci_disable_device() is called), right?

Oops! Right, the documentation says .error_detected is _always_ called before .slot_reset. So, this patch is not correct. Please don't merge this.

Quoted 1 line

From Documentation/pci-error-recovery.txt:

STEP 1: Notification
--------------------
Platform calls the error_detected() callback on every instance of every driver affected by the error.
...

If any driver requested a slot reset (by returning PCI_ERS_RESULT_NEED_RESET), then recovery proceeds to STEP 4 (Slot Reset).

Quoted 3 lines

The pci_reenable_device() will call enable only if the device was enabled before and would not be enabled if the device were disabled. Is this the intended behavior?

Yes, you are right. And no it isn't.

Thanks,

Brandon

Quoted 88 lines
Quoted 2 lines

-----Original Message-----
From: [email protected]

[mailto:[email protected]]

Quoted 38 lines

On Behalf Of Brandon Philips
Sent: Thursday, August 02, 2007 3:44 PM
To: [email protected]
Cc: [email protected]; Brandon Philips
Subject: [patch 1/5][RFC] NET: Change pci_enable_device topci_reenable_device to keep device enable balance

On a slot_reset event pci_disable_device() is never called so calling pci_enable_device() will unbalance the enable count.

Signed-off-by Brandon Philips <[email protected]>
---
 drivers/net/e100.c             |    2 +-
 drivers/net/e1000/e1000_main.c |    2 +-
 drivers/net/ixgb/ixgb_main.c   |    2 +-
 drivers/net/s2io.c             |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/net/e100.c
===================================================================

drivers/net/e100.c +1 -1
@@ -2828,7 +2828,7 @@ static pci_ers_result_t e100_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct nic *nic = netdev_priv(netdev);
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "e100: Cannot re-enable PCI device after
 reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;

}
Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================

drivers/net/e1000/e1000_main.c static pci_ers_result_t e1000_io_slot_re · line 5274

Quoted 6 lines
drivers/net/e1000/e1000_main.c +1 -1
@@ -5270,7 +5270,7 @@ static pci_ers_result_t e1000_io_slot_re
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct e1000_adapter *adapter = netdev->priv;
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "e1000: Cannot re-enable PCI device

after

Quoted 33 lines
drivers/net/e1000/e1000_main.c +0 -0
 reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;

}
Index: linux-2.6/drivers/net/ixgb/ixgb_main.c
===================================================================

drivers/net/ixgb/ixgb_main.c +1 -1
@@ -2294,7 +2294,7 @@ static pci_ers_result_t ixgb_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
 
-	if(pci_enable_device(pdev)) {
+	if(pci_reenable_device(pdev)) {
 		DPRINTK(PROBE, ERR, "Cannot re-enable PCI device after
 reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;

}
Index: linux-2.6/drivers/net/s2io.c
===================================================================

drivers/net/s2io.c +1 -3
@@ -7833,7 +7833,7 @@ static pci_ers_result_t s2io_io_slot_res
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct s2io_nic *sp = netdev->priv;
 
-	if (pci_enable_device(pdev)) {
+	if (pci_reenable_device(pdev)) {
 		printk(KERN_ERR "s2io: "
 		       "Cannot re-enable PCI device after reset.\n");
 		return PCI_ERS_RESULT_DISCONNECT;
 
--
-
Quoted 3 lines

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

-
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

#4 lore raw

Re: [patch 1/5][RFC] NET: Change pci_enable_device topci_reenable_device to keep device enable balance

Quoted 16 lines · Brandon Philips
Quoted 2 lines · Ramkrishna Vepa

Before slot_reset event is called io_error_detected could be called (where pci_disable_device() is called), right?

Oops! Right, the documentation says .error_detected is _always_ called before .slot_reset. So, this patch is not correct. Please don't merge this.

From Documentation/pci-error-recovery.txt:

STEP 1: Notification
--------------------
Platform calls the error_detected() callback on every instance of every driver affected by the error.
...

Quoted 8 lines

If any driver requested a slot reset (by returning PCI_ERS_RESULT_NEED_RESET), then recovery proceeds to STEP 4 (Slot Reset).

Quoted 3 lines

The pci_reenable_device() will call enable only if the device was enabled before and would not be enabled if the device were disabled. Is this the intended behavior?

Yes, you are right. And no it isn't.

it would be great if you can convince Linas Vepstas to review and ack your patch - he's the one who knows the error handling code the best.

Auke