* [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives
@ 2007-02-20 0:57 Chris Snook
2007-02-20 1:03 ` [PATCH 1/3] remove irq_sem from atl1 Chris Snook
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Chris Snook @ 2007-02-20 0:57 UTC (permalink / raw)
To: jeff, jacliburn, csnook, john.ronciak, jesse.brandeburg, auke-jan.h.kok
Cc: netdev
Hey folks --
While digging through the atl1 source, I was troubled by the code using
irq_sem. I did some digging and found the same code in e1000 and ixgb. I'm
not entirely sure what it was originally intended to do, but it doesn't seem
to be doing anything useful now, except possibly locking interrupts off if
NAPI is flipped on and off enough times to cause an integer overflow.
The following patches completely remove irq_sem from each of the drivers.
This has been tested successfully on atl1 and e1000. If someone would like
to send me ixgb hardware I'd be glad to test that, otherwise you'll have to
test it yourself. :)
-- Chris
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] remove irq_sem from atl1
2007-02-20 0:57 [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Chris Snook
@ 2007-02-20 1:03 ` Chris Snook
2007-02-27 9:32 ` Jeff Garzik
2007-02-20 1:06 ` [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Auke Kok
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Chris Snook @ 2007-02-20 1:03 UTC (permalink / raw)
To: jeff, jacliburn, csnook, john.ronciak, jesse.brandeburg, auke-jan.h.kok
Cc: netdev
From: Chris Snook <csnook@redhat.com>
Remove unnecessary irq_sem code from atl1 driver. Tested with no problems.
Signed-off-by: Chris Snook <csnook@redhat.com>
Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
--
diff -urp linux-2.6.20-git14.orig/drivers/net/atl1/atl1.h
linux-2.6.20-git14/drivers/net/atl1/atl1.h
--- linux-2.6.20-git14.orig/drivers/net/atl1/atl1.h 2007-02-19
14:32:15.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/atl1/atl1.h 2007-02-19 15:10:07.000000000
-0500
@@ -236,7 +236,6 @@ struct atl1_adapter {
u16 link_speed;
u16 link_duplex;
spinlock_t lock;
- atomic_t irq_sem;
struct work_struct tx_timeout_task;
struct work_struct link_chg_task;
struct work_struct pcie_dma_to_rst_task;
diff -urp linux-2.6.20-git14.orig/drivers/net/atl1/atl1_main.c
linux-2.6.20-git14/drivers/net/atl1/atl1_main.c
--- linux-2.6.20-git14.orig/drivers/net/atl1/atl1_main.c 2007-02-19
14:32:15.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/atl1/atl1_main.c 2007-02-19
15:10:44.000000000 -0500
@@ -163,7 +163,6 @@ static int __devinit atl1_sw_init(struct
hw->cmb_tx_timer = 1; /* about 2us */
hw->smb_timer = 100000; /* about 200ms */
- atomic_set(&adapter->irq_sem, 0);
spin_lock_init(&adapter->lock);
spin_lock_init(&adapter->mb_lock);
@@ -272,8 +271,7 @@ err_nomem:
*/
static void atl1_irq_enable(struct atl1_adapter *adapter)
{
- if (likely(!atomic_dec_and_test(&adapter->irq_sem)))
- iowrite32(IMR_NORMAL_MASK, adapter->hw.hw_addr + REG_IMR);
+ iowrite32(IMR_NORMAL_MASK, adapter->hw.hw_addr + REG_IMR);
}
static void atl1_clear_phy_int(struct atl1_adapter *adapter)
@@ -1205,7 +1203,6 @@ static u32 atl1_configure(struct atl1_ad
*/
static void atl1_irq_disable(struct atl1_adapter *adapter)
{
- atomic_inc(&adapter->irq_sem);
iowrite32(0, adapter->hw.hw_addr + REG_IMR);
ioread32(adapter->hw.hw_addr + REG_IMR);
synchronize_irq(adapter->pdev->irq);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives
2007-02-20 0:57 [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Chris Snook
2007-02-20 1:03 ` [PATCH 1/3] remove irq_sem from atl1 Chris Snook
@ 2007-02-20 1:06 ` Auke Kok
2007-02-20 1:10 ` [PATCH 2/3] remove irq_sem from e1000 Chris Snook
2007-02-20 1:12 ` [PATCH 3/3] remove irq_sem from ixgb Chris Snook
3 siblings, 0 replies; 9+ messages in thread
From: Auke Kok @ 2007-02-20 1:06 UTC (permalink / raw)
To: Chris Snook; +Cc: jeff, jacliburn, john.ronciak, jesse.brandeburg, netdev
Chris Snook wrote:
> Hey folks --
>
> While digging through the atl1 source, I was troubled by the code using
> irq_sem. I did some digging and found the same code in e1000 and ixgb. I'm
> not entirely sure what it was originally intended to do, but it doesn't seem
> to be doing anything useful now, except possibly locking interrupts off if
> NAPI is flipped on and off enough times to cause an integer overflow.
>
> The following patches completely remove irq_sem from each of the drivers.
> This has been tested successfully on atl1 and e1000. If someone would like
> to send me ixgb hardware I'd be glad to test that, otherwise you'll have to
> test it yourself. :)
>
> -- Chris
I'm not yet seeing patches 1/3 appear, but I'll certainly take a look at them
and have them tested in our labs once they appear for e1000 and ixgb.
Cheers,
Auke
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] remove irq_sem from e1000
2007-02-20 0:57 [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Chris Snook
2007-02-20 1:03 ` [PATCH 1/3] remove irq_sem from atl1 Chris Snook
2007-02-20 1:06 ` [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Auke Kok
@ 2007-02-20 1:10 ` Chris Snook
2007-04-09 17:54 ` Kok, Auke
2007-02-20 1:12 ` [PATCH 3/3] remove irq_sem from ixgb Chris Snook
3 siblings, 1 reply; 9+ messages in thread
From: Chris Snook @ 2007-02-20 1:10 UTC (permalink / raw)
To: jeff, jacliburn, csnook, john.ronciak, jesse.brandeburg, auke-jan.h.kok
Cc: netdev
From: Chris Snook <csnook@redhat.com>
Remove unnecessary irq_sem accounting from e1000. Tested with no problems.
Signed-off-by: Chris Snook <csnook@redhat.com>
--
diff -urp linux-2.6.20-git14.orig/drivers/net/e1000/e1000.h
linux-2.6.20-git14/drivers/net/e1000/e1000.h
--- linux-2.6.20-git14.orig/drivers/net/e1000/e1000.h 2007-02-19
14:32:15.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/e1000/e1000.h 2007-02-19 15:07:37.000000000
-0500
@@ -252,7 +252,6 @@ struct e1000_adapter {
#ifdef CONFIG_E1000_NAPI
spinlock_t tx_queue_lock;
#endif
- atomic_t irq_sem;
unsigned int total_tx_bytes;
unsigned int total_tx_packets;
unsigned int total_rx_bytes;
diff -urp linux-2.6.20-git14.orig/drivers/net/e1000/e1000_main.c
linux-2.6.20-git14/drivers/net/e1000/e1000_main.c
--- linux-2.6.20-git14.orig/drivers/net/e1000/e1000_main.c 2007-02-19
14:32:15.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/e1000/e1000_main.c 2007-02-19
15:09:28.000000000 -0500
@@ -349,7 +349,6 @@ static void e1000_free_irq(struct e1000_
static void
e1000_irq_disable(struct e1000_adapter *adapter)
{
- atomic_inc(&adapter->irq_sem);
E1000_WRITE_REG(&adapter->hw, IMC, ~0);
E1000_WRITE_FLUSH(&adapter->hw);
synchronize_irq(adapter->pdev->irq);
@@ -363,10 +362,8 @@ e1000_irq_disable(struct e1000_adapter *
static void
e1000_irq_enable(struct e1000_adapter *adapter)
{
- if (likely(atomic_dec_and_test(&adapter->irq_sem))) {
- E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
- E1000_WRITE_FLUSH(&adapter->hw);
- }
+ E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
+ E1000_WRITE_FLUSH(&adapter->hw);
}
static void
@@ -1336,7 +1333,6 @@ e1000_sw_init(struct e1000_adapter *adap
spin_lock_init(&adapter->tx_queue_lock);
#endif
- atomic_set(&adapter->irq_sem, 1);
spin_lock_init(&adapter->stats_lock);
set_bit(__E1000_DOWN, &adapter->flags);
@@ -3758,11 +3754,6 @@ e1000_intr_msi(int irq, void *data)
#endif
uint32_t icr = E1000_READ_REG(hw, ICR);
-#ifdef CONFIG_E1000_NAPI
- /* read ICR disables interrupts using IAM, so keep up with our
- * enable/disable accounting */
- atomic_inc(&adapter->irq_sem);
-#endif
if (icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC)) {
hw->get_link_status = 1;
/* 80003ES2LAN workaround-- For packet buffer work-around on
@@ -3832,13 +3823,6 @@ e1000_intr(int irq, void *data)
if (unlikely(hw->mac_type >= e1000_82571 &&
!(icr & E1000_ICR_INT_ASSERTED)))
return IRQ_NONE;
-
- /* Interrupt Auto-Mask...upon reading ICR,
- * interrupts are masked. No need for the
- * IMC write, but it does mean we should
- * account for it ASAP. */
- if (likely(hw->mac_type >= e1000_82571))
- atomic_inc(&adapter->irq_sem);
#endif
if (unlikely(icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC))) {
@@ -3862,7 +3846,6 @@ e1000_intr(int irq, void *data)
#ifdef CONFIG_E1000_NAPI
if (unlikely(hw->mac_type < e1000_82571)) {
/* disable interrupts, without the synchronize_irq bit */
- atomic_inc(&adapter->irq_sem);
E1000_WRITE_REG(hw, IMC, ~0);
E1000_WRITE_FLUSH(hw);
}
@@ -3888,7 +3871,6 @@ e1000_intr(int irq, void *data)
* de-assertion state.
*/
if (hw->mac_type == e1000_82547 || hw->mac_type == e1000_82547_rev_2) {
- atomic_inc(&adapter->irq_sem);
E1000_WRITE_REG(hw, IMC, ~0);
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] remove irq_sem from ixgb
2007-02-20 0:57 [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Chris Snook
` (2 preceding siblings ...)
2007-02-20 1:10 ` [PATCH 2/3] remove irq_sem from e1000 Chris Snook
@ 2007-02-20 1:12 ` Chris Snook
3 siblings, 0 replies; 9+ messages in thread
From: Chris Snook @ 2007-02-20 1:12 UTC (permalink / raw)
To: jeff, jacliburn, csnook, john.ronciak, jesse.brandeburg, auke-jan.h.kok
Cc: netdev
From: Chris Snook <csnook@redhat.com>
Remove irq_sem from ixgb. Currently untested, but similar to tested patches
on atl1 and e1000.
Signed-off-by: Chris Snook <csnook@redhat.com>
--
diff -urp linux-2.6.20-git14.orig/drivers/net/ixgb/ixgb.h
linux-2.6.20-git14/drivers/net/ixgb/ixgb.h
--- linux-2.6.20-git14.orig/drivers/net/ixgb/ixgb.h 2007-02-19
14:32:16.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/ixgb/ixgb.h 2007-02-19 15:04:50.000000000
-0500
@@ -161,7 +161,6 @@ struct ixgb_adapter {
uint16_t link_speed;
uint16_t link_duplex;
spinlock_t tx_lock;
- atomic_t irq_sem;
struct work_struct tx_timeout_task;
struct timer_list blink_timer;
diff -urp linux-2.6.20-git14.orig/drivers/net/ixgb/ixgb_main.c
linux-2.6.20-git14/drivers/net/ixgb/ixgb_main.c
--- linux-2.6.20-git14.orig/drivers/net/ixgb/ixgb_main.c 2007-02-19
14:32:16.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/ixgb/ixgb_main.c 2007-02-19
15:06:52.000000000 -0500
@@ -201,7 +201,6 @@ module_exit(ixgb_exit_module);
static void
ixgb_irq_disable(struct ixgb_adapter *adapter)
{
- atomic_inc(&adapter->irq_sem);
IXGB_WRITE_REG(&adapter->hw, IMC, ~0);
IXGB_WRITE_FLUSH(&adapter->hw);
synchronize_irq(adapter->pdev->irq);
@@ -215,12 +214,10 @@ ixgb_irq_disable(struct ixgb_adapter *ad
static void
ixgb_irq_enable(struct ixgb_adapter *adapter)
{
- if(atomic_dec_and_test(&adapter->irq_sem)) {
- IXGB_WRITE_REG(&adapter->hw, IMS,
- IXGB_INT_RXT0 | IXGB_INT_RXDMT0 | IXGB_INT_TXDW |
- IXGB_INT_LSC);
- IXGB_WRITE_FLUSH(&adapter->hw);
- }
+ IXGB_WRITE_REG(&adapter->hw, IMS,
+ IXGB_INT_RXT0 | IXGB_INT_RXDMT0 | IXGB_INT_TXDW |
+ IXGB_INT_LSC);
+ IXGB_WRITE_FLUSH(&adapter->hw);
}
int
@@ -584,7 +581,6 @@ ixgb_sw_init(struct ixgb_adapter *adapte
/* enable flow control to be programmed */
hw->fc.send_xon = 1;
- atomic_set(&adapter->irq_sem, 1);
spin_lock_init(&adapter->tx_lock);
return 0;
@@ -1755,7 +1751,6 @@ ixgb_intr(int irq, void *data)
of the posted write is intentionally left out.
*/
- atomic_inc(&adapter->irq_sem);
IXGB_WRITE_REG(&adapter->hw, IMC, ~0);
__netif_rx_schedule(netdev);
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] remove irq_sem from atl1
2007-02-20 1:03 ` [PATCH 1/3] remove irq_sem from atl1 Chris Snook
@ 2007-02-27 9:32 ` Jeff Garzik
2007-02-27 17:28 ` Chris Snook
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Garzik @ 2007-02-27 9:32 UTC (permalink / raw)
To: Chris Snook
Cc: jacliburn, john.ronciak, jesse.brandeburg, auke-jan.h.kok, netdev
Chris Snook wrote:
> From: Chris Snook <csnook@redhat.com>
>
> Remove unnecessary irq_sem code from atl1 driver. Tested with no problems.
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
> Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
ACK, but patch does not apply:
Applying 'remove irq_sem from atl1'
error: patch fragment without header at line 7: @@ -236,7 +236,6 @@
struct atl1_adapter {
error: patch fragment without header at line 21: @@ -163,7 +163,6 @@
static int __devinit atl1_sw_init(struct
error: patch fragment without header at line 29: @@ -272,8 +271,7 @@
err_nomem:
error: patch fragment without header at line 39: @@ -1205,7 +1203,6 @@
static u32 atl1_configure(struct atl1_ad
error: No changes
Patch failed at 0004.
When you have resolved this problem run "git-am --resolved".
If you would prefer to skip this patch, instead run "git-am --skip".
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] remove irq_sem from atl1
2007-02-27 9:32 ` Jeff Garzik
@ 2007-02-27 17:28 ` Chris Snook
0 siblings, 0 replies; 9+ messages in thread
From: Chris Snook @ 2007-02-27 17:28 UTC (permalink / raw)
To: Jeff Garzik; +Cc: jacliburn, netdev
[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]
Jeff Garzik wrote:
> Chris Snook wrote:
>> From: Chris Snook <csnook@redhat.com>
>>
>> Remove unnecessary irq_sem code from atl1 driver. Tested with no
>> problems.
>>
>> Signed-off-by: Chris Snook <csnook@redhat.com>
>> Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
>
> ACK, but patch does not apply:
>
>
> Applying 'remove irq_sem from atl1'
>
> error: patch fragment without header at line 7: @@ -236,7 +236,6 @@
> struct atl1_adapter {
> error: patch fragment without header at line 21: @@ -163,7 +163,6 @@
> static int __devinit atl1_sw_init(struct
> error: patch fragment without header at line 29: @@ -272,8 +271,7 @@
> err_nomem:
> error: patch fragment without header at line 39: @@ -1205,7 +1203,6 @@
> static u32 atl1_configure(struct atl1_ad
> error: No changes
> Patch failed at 0004.
> When you have resolved this problem run "git-am --resolved".
> If you would prefer to skip this patch, instead run "git-am --skip".
Huh, looks like my mailer line-wrapped the headers. Patch didn't
complain, but I guess git is pickier. I've attached the patch to avoid
text mangling. Were there any problems with the e1000 and ixgb patches?
-- Chris
[-- Attachment #2: remove-irq_sem-atl1.patch --]
[-- Type: text/x-patch, Size: 1691 bytes --]
diff -urp linux-2.6.20-git14.orig/drivers/net/atl1/atl1.h linux-2.6.20-git14/drivers/net/atl1/atl1.h
--- linux-2.6.20-git14.orig/drivers/net/atl1/atl1.h 2007-02-19 14:32:15.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/atl1/atl1.h 2007-02-19 15:10:07.000000000 -0500
@@ -236,7 +236,6 @@ struct atl1_adapter {
u16 link_speed;
u16 link_duplex;
spinlock_t lock;
- atomic_t irq_sem;
struct work_struct tx_timeout_task;
struct work_struct link_chg_task;
struct work_struct pcie_dma_to_rst_task;
diff -urp linux-2.6.20-git14.orig/drivers/net/atl1/atl1_main.c linux-2.6.20-git14/drivers/net/atl1/atl1_main.c
--- linux-2.6.20-git14.orig/drivers/net/atl1/atl1_main.c 2007-02-19 14:32:15.000000000 -0500
+++ linux-2.6.20-git14/drivers/net/atl1/atl1_main.c 2007-02-19 15:10:44.000000000 -0500
@@ -163,7 +163,6 @@ static int __devinit atl1_sw_init(struct
hw->cmb_tx_timer = 1; /* about 2us */
hw->smb_timer = 100000; /* about 200ms */
- atomic_set(&adapter->irq_sem, 0);
spin_lock_init(&adapter->lock);
spin_lock_init(&adapter->mb_lock);
@@ -272,8 +271,7 @@ err_nomem:
*/
static void atl1_irq_enable(struct atl1_adapter *adapter)
{
- if (likely(!atomic_dec_and_test(&adapter->irq_sem)))
- iowrite32(IMR_NORMAL_MASK, adapter->hw.hw_addr + REG_IMR);
+ iowrite32(IMR_NORMAL_MASK, adapter->hw.hw_addr + REG_IMR);
}
static void atl1_clear_phy_int(struct atl1_adapter *adapter)
@@ -1205,7 +1203,6 @@ static u32 atl1_configure(struct atl1_ad
*/
static void atl1_irq_disable(struct atl1_adapter *adapter)
{
- atomic_inc(&adapter->irq_sem);
iowrite32(0, adapter->hw.hw_addr + REG_IMR);
ioread32(adapter->hw.hw_addr + REG_IMR);
synchronize_irq(adapter->pdev->irq);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] remove irq_sem from e1000
2007-02-20 1:10 ` [PATCH 2/3] remove irq_sem from e1000 Chris Snook
@ 2007-04-09 17:54 ` Kok, Auke
2007-04-09 20:34 ` Chris Snook
0 siblings, 1 reply; 9+ messages in thread
From: Kok, Auke @ 2007-04-09 17:54 UTC (permalink / raw)
To: Chris Snook
Cc: jeff, jacliburn, john.ronciak, jesse.brandeburg, auke-jan.h.kok, netdev
Chris Snook wrote:
> From: Chris Snook <csnook@redhat.com>
>
> Remove unnecessary irq_sem accounting from e1000. Tested with no problems.
the major problem with this is that one of the e1000 parts (82547) still
requires the irq_sem. I doubt that you tested that card specifically. I'm still
not convinced that this patch is therefore a good idea. the patch for ixgb is
another thing, but for e1000 we definately want to keep the irq_sem around until
we get some definitive testing on all adapters.
So, this patch will stay under suspicion a bit longer. Same for the ixgb version.
Auke
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
> --
> diff -urp linux-2.6.20-git14.orig/drivers/net/e1000/e1000.h
> linux-2.6.20-git14/drivers/net/e1000/e1000.h
> --- linux-2.6.20-git14.orig/drivers/net/e1000/e1000.h 2007-02-19
> 14:32:15.000000000 -0500
> +++ linux-2.6.20-git14/drivers/net/e1000/e1000.h 2007-02-19 15:07:37.000000000
> -0500
> @@ -252,7 +252,6 @@ struct e1000_adapter {
> #ifdef CONFIG_E1000_NAPI
> spinlock_t tx_queue_lock;
> #endif
> - atomic_t irq_sem;
> unsigned int total_tx_bytes;
> unsigned int total_tx_packets;
> unsigned int total_rx_bytes;
> diff -urp linux-2.6.20-git14.orig/drivers/net/e1000/e1000_main.c
> linux-2.6.20-git14/drivers/net/e1000/e1000_main.c
> --- linux-2.6.20-git14.orig/drivers/net/e1000/e1000_main.c 2007-02-19
> 14:32:15.000000000 -0500
> +++ linux-2.6.20-git14/drivers/net/e1000/e1000_main.c 2007-02-19
> 15:09:28.000000000 -0500
> @@ -349,7 +349,6 @@ static void e1000_free_irq(struct e1000_
> static void
> e1000_irq_disable(struct e1000_adapter *adapter)
> {
> - atomic_inc(&adapter->irq_sem);
> E1000_WRITE_REG(&adapter->hw, IMC, ~0);
> E1000_WRITE_FLUSH(&adapter->hw);
> synchronize_irq(adapter->pdev->irq);
> @@ -363,10 +362,8 @@ e1000_irq_disable(struct e1000_adapter *
> static void
> e1000_irq_enable(struct e1000_adapter *adapter)
> {
> - if (likely(atomic_dec_and_test(&adapter->irq_sem))) {
> - E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
> - E1000_WRITE_FLUSH(&adapter->hw);
> - }
> + E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
> + E1000_WRITE_FLUSH(&adapter->hw);
> }
>
> static void
> @@ -1336,7 +1333,6 @@ e1000_sw_init(struct e1000_adapter *adap
> spin_lock_init(&adapter->tx_queue_lock);
> #endif
>
> - atomic_set(&adapter->irq_sem, 1);
> spin_lock_init(&adapter->stats_lock);
>
> set_bit(__E1000_DOWN, &adapter->flags);
> @@ -3758,11 +3754,6 @@ e1000_intr_msi(int irq, void *data)
> #endif
> uint32_t icr = E1000_READ_REG(hw, ICR);
>
> -#ifdef CONFIG_E1000_NAPI
> - /* read ICR disables interrupts using IAM, so keep up with our
> - * enable/disable accounting */
> - atomic_inc(&adapter->irq_sem);
> -#endif
> if (icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC)) {
> hw->get_link_status = 1;
> /* 80003ES2LAN workaround-- For packet buffer work-around on
> @@ -3832,13 +3823,6 @@ e1000_intr(int irq, void *data)
> if (unlikely(hw->mac_type >= e1000_82571 &&
> !(icr & E1000_ICR_INT_ASSERTED)))
> return IRQ_NONE;
> -
> - /* Interrupt Auto-Mask...upon reading ICR,
> - * interrupts are masked. No need for the
> - * IMC write, but it does mean we should
> - * account for it ASAP. */
> - if (likely(hw->mac_type >= e1000_82571))
> - atomic_inc(&adapter->irq_sem);
> #endif
>
> if (unlikely(icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC))) {
> @@ -3862,7 +3846,6 @@ e1000_intr(int irq, void *data)
> #ifdef CONFIG_E1000_NAPI
> if (unlikely(hw->mac_type < e1000_82571)) {
> /* disable interrupts, without the synchronize_irq bit */
> - atomic_inc(&adapter->irq_sem);
> E1000_WRITE_REG(hw, IMC, ~0);
> E1000_WRITE_FLUSH(hw);
> }
> @@ -3888,7 +3871,6 @@ e1000_intr(int irq, void *data)
> * de-assertion state.
> */
> if (hw->mac_type == e1000_82547 || hw->mac_type == e1000_82547_rev_2) {
> - atomic_inc(&adapter->irq_sem);
> E1000_WRITE_REG(hw, IMC, ~0);
> }
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] remove irq_sem from e1000
2007-04-09 17:54 ` Kok, Auke
@ 2007-04-09 20:34 ` Chris Snook
0 siblings, 0 replies; 9+ messages in thread
From: Chris Snook @ 2007-04-09 20:34 UTC (permalink / raw)
To: Kok, Auke; +Cc: jeff, jacliburn, john.ronciak, jesse.brandeburg, netdev
Kok, Auke wrote:
> Chris Snook wrote:
>> From: Chris Snook <csnook@redhat.com>
>>
>> Remove unnecessary irq_sem accounting from e1000. Tested with no
>> problems.
>
>
> the major problem with this is that one of the e1000 parts (82547) still
> requires the irq_sem. I doubt that you tested that card specifically.
> I'm still not convinced that this patch is therefore a good idea. the
> patch for ixgb is another thing, but for e1000 we definately want to
> keep the irq_sem around until we get some definitive testing on all
> adapters.
>
> So, this patch will stay under suspicion a bit longer. Same for the ixgb
> version.
>
> Auke
Requires? I acknowledge that the code is used in a special codepath for
those cards, but it appears to be used in the same unnecessary way it is
used everywhere else.
My testing was limited to an onboard NIC on a fairly old box I don't
have access to anymore, as well as the e1000-derived atl1 driver. My
primary justification is code inspection. The code just doesn't do
anything productive.
-- Chris
>>
>> Signed-off-by: Chris Snook <csnook@redhat.com>
>> --
>> diff -urp linux-2.6.20-git14.orig/drivers/net/e1000/e1000.h
>> linux-2.6.20-git14/drivers/net/e1000/e1000.h
>> --- linux-2.6.20-git14.orig/drivers/net/e1000/e1000.h 2007-02-19
>> 14:32:15.000000000 -0500
>> +++ linux-2.6.20-git14/drivers/net/e1000/e1000.h 2007-02-19
>> 15:07:37.000000000
>> -0500
>> @@ -252,7 +252,6 @@ struct e1000_adapter {
>> #ifdef CONFIG_E1000_NAPI
>> spinlock_t tx_queue_lock;
>> #endif
>> - atomic_t irq_sem;
>> unsigned int total_tx_bytes;
>> unsigned int total_tx_packets;
>> unsigned int total_rx_bytes;
>> diff -urp linux-2.6.20-git14.orig/drivers/net/e1000/e1000_main.c
>> linux-2.6.20-git14/drivers/net/e1000/e1000_main.c
>> --- linux-2.6.20-git14.orig/drivers/net/e1000/e1000_main.c 2007-02-19
>> 14:32:15.000000000 -0500
>> +++ linux-2.6.20-git14/drivers/net/e1000/e1000_main.c 2007-02-19
>> 15:09:28.000000000 -0500
>> @@ -349,7 +349,6 @@ static void e1000_free_irq(struct e1000_
>> static void
>> e1000_irq_disable(struct e1000_adapter *adapter)
>> {
>> - atomic_inc(&adapter->irq_sem);
>> E1000_WRITE_REG(&adapter->hw, IMC, ~0);
>> E1000_WRITE_FLUSH(&adapter->hw);
>> synchronize_irq(adapter->pdev->irq);
>> @@ -363,10 +362,8 @@ e1000_irq_disable(struct e1000_adapter *
>> static void
>> e1000_irq_enable(struct e1000_adapter *adapter)
>> {
>> - if (likely(atomic_dec_and_test(&adapter->irq_sem))) {
>> - E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
>> - E1000_WRITE_FLUSH(&adapter->hw);
>> - }
>> + E1000_WRITE_REG(&adapter->hw, IMS, IMS_ENABLE_MASK);
>> + E1000_WRITE_FLUSH(&adapter->hw);
>> }
>>
>> static void
>> @@ -1336,7 +1333,6 @@ e1000_sw_init(struct e1000_adapter *adap
>> spin_lock_init(&adapter->tx_queue_lock);
>> #endif
>>
>> - atomic_set(&adapter->irq_sem, 1);
>> spin_lock_init(&adapter->stats_lock);
>>
>> set_bit(__E1000_DOWN, &adapter->flags);
>> @@ -3758,11 +3754,6 @@ e1000_intr_msi(int irq, void *data)
>> #endif
>> uint32_t icr = E1000_READ_REG(hw, ICR);
>>
>> -#ifdef CONFIG_E1000_NAPI
>> - /* read ICR disables interrupts using IAM, so keep up with our
>> - * enable/disable accounting */
>> - atomic_inc(&adapter->irq_sem);
>> -#endif
>> if (icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC)) {
>> hw->get_link_status = 1;
>> /* 80003ES2LAN workaround-- For packet buffer work-around on
>> @@ -3832,13 +3823,6 @@ e1000_intr(int irq, void *data)
>> if (unlikely(hw->mac_type >= e1000_82571 &&
>> !(icr & E1000_ICR_INT_ASSERTED)))
>> return IRQ_NONE;
>> -
>> - /* Interrupt Auto-Mask...upon reading ICR,
>> - * interrupts are masked. No need for the
>> - * IMC write, but it does mean we should
>> - * account for it ASAP. */
>> - if (likely(hw->mac_type >= e1000_82571))
>> - atomic_inc(&adapter->irq_sem);
>> #endif
>>
>> if (unlikely(icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC))) {
>> @@ -3862,7 +3846,6 @@ e1000_intr(int irq, void *data)
>> #ifdef CONFIG_E1000_NAPI
>> if (unlikely(hw->mac_type < e1000_82571)) {
>> /* disable interrupts, without the synchronize_irq bit */
>> - atomic_inc(&adapter->irq_sem);
>> E1000_WRITE_REG(hw, IMC, ~0);
>> E1000_WRITE_FLUSH(hw);
>> }
>> @@ -3888,7 +3871,6 @@ e1000_intr(int irq, void *data)
>> * de-assertion state.
>> */
>> if (hw->mac_type == e1000_82547 || hw->mac_type ==
>> e1000_82547_rev_2) {
>> - atomic_inc(&adapter->irq_sem);
>> E1000_WRITE_REG(hw, IMC, ~0);
>> }
>>
>> -
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-04-09 20:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-20 0:57 [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Chris Snook
2007-02-20 1:03 ` [PATCH 1/3] remove irq_sem from atl1 Chris Snook
2007-02-27 9:32 ` Jeff Garzik
2007-02-27 17:28 ` Chris Snook
2007-02-20 1:06 ` [PATCH 0/3] remove irq_sem cruft from e1000 and derivatives Auke Kok
2007-02-20 1:10 ` [PATCH 2/3] remove irq_sem from e1000 Chris Snook
2007-04-09 17:54 ` Kok, Auke
2007-04-09 20:34 ` Chris Snook
2007-02-20 1:12 ` [PATCH 3/3] remove irq_sem from ixgb Chris Snook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.