Add missing memory barriers to clean_rx_irq functions in Intel Drivers
Subsystems (2)
PATCH 1/1 Add missing memory barriers to clean_rx_irq functions in Intel Drivers
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf I've also added a missing barrier to the clean_tx_irq path because I missed it in my last patch.
Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
drivers/net/e1000/e1000_main.c +2 -0
@@ -3638,6 +3638,7 @@ static bool e1000_clean_jumbo_rx_irq(strif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
@@ -3844,6 +3845,7 @@ static bool e1000_clean_rx_irq(struct e1if (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
drivers/net/e1000e/netdev.c +3 -0
@@ -774,6 +774,7 @@ static bool e1000_clean_rx_irq(struct e1if (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
@@ -1081,6 +1082,7 @@ static bool e1000_clean_rx_irq_ps(structbreak ; (*work_done)++; skb = buffer_info->skb; +rmb ();/* read descriptor and rx_buffer_info after status DD */ /* in the packet split case this is header only */ prefetch (skb->data -NET_IP_ALIGN );
@@ -1280,6 +1282,7 @@ static bool e1000_clean_jumbo_rx_irq(strif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
drivers/net/ixgb/ixgb_main.c +1 -0
@@ -1977,6 +1977,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *abreak ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb; buffer_info->skb =NULL ;
drivers/net/ixgbevf/ixgbevf_main.c +2 -0
@@ -231,6 +231,7 @@ static bool ixgbevf_clean_tx_irq(structwhile ((eop_desc->wb.status &cpu_to_le32 (IXGBE_TXD_STAT_DD )) && (count < tx_ring->work_limit)) {bool cleaned = false; +rmb ();/* read buffer_info after eop_desc */ for ( ; !cleaned; count++) {struct sk_buff *skb; tx_desc =IXGBE_TX_DESC_ADV (*tx_ring, i);
@@ -518,6 +519,7 @@ static bool ixgbevf_clean_rx_irq(structbreak ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ if (adapter->flags &IXGBE_FLAG_RX_PS_ENABLED ) { hdr_info =le16_to_cpu (ixgbevf_get_hdr_info (rx_desc)); len = (hdr_info & IXGBE_RXDADV_HDRBUFLEN_MASK) >>
Quoted 15 lines · Sonny Rao
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf I've also added a missing barrier to the
clean_tx_irqpath because I missed it in my last patch.Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
Quoted 4 lines
Signed-off-by Milton Miller <[email protected]>Signed-off-by Sonny Rao <[email protected]>cc stable <[email protected]>
I already have a similar patch in my queue from you Sonny, although I see that this patch has made a few more changes. Is this version 2?
Signature · 2 lines
Cheers, Jeff
Quoted 16 lines · Jeff Kirsher
Quoted 15 lines · Sonny Rao
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf I've also added a missing barrier to the
clean_tx_irqpath because I missed it in my last patch.Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Milton Miller <[email protected]>Signed-off-by Sonny Rao <[email protected]>cc stable <[email protected]>I already have a similar patch in my queue from you Sonny, although I see that this patch has made a few more changes. Is this version 2?
Well, the previous one was for the clean_tx_irq functions this one is for the clean_rx_irq functions. I'd gotten the two confused when I referenced Anton's original patch -- which was also a clean_rx_irq patch. So they are touching different code paths but fixing similar problems.
Signature · 1 line
Sonny Rao, LTC OzLabs, BML team
Quoted 32 lines · Sonny Rao
Quoted 16 lines · Jeff Kirsher
Quoted 15 lines · Sonny Rao
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf I've also added a missing barrier to the
clean_tx_irqpath because I missed it in my last patch.Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Milton Miller <[email protected]>Signed-off-by Sonny Rao <[email protected]>cc stable <[email protected]>I already have a similar patch in my queue from you Sonny, although I see that this patch has made a few more changes. Is this version 2?
Well, the previous one was for the
clean_tx_irqfunctions this one is for theclean_rx_irqfunctions. I'd gotten the two confused when I referenced Anton's original patch -- which was also aclean_rx_irqpatch. So they are touching different code paths but fixing similar problems.
Quoted 3 lines
--
Sonny Rao, LTC OzLabs, BML team
--
Ok, just wanted to make sure. In the first patch (which I already have in my queue) that cleans up clean_tx_irq, your missing igb driver as well.
Signature · 2 lines
Cheers, Jeff
PATCH v2 1/1 Add missing memory barriers to clean_rx_irq functions in Intel Drivers
In reply to Sonny RaoAdd missing memory barriers to clean_rx_irq functions in Intel Drivers
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf I've also added a missing barrier to the clean_tx_irq path because I missed it in my last patch.
Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
v2: I missed the e100 the first time
drivers/net/e1000/e1000_main.c +2 -0
@@ -3638,6 +3638,7 @@ static bool e1000_clean_jumbo_rx_irq(strif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
@@ -3844,6 +3845,7 @@ static bool e1000_clean_rx_irq(struct e1if (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
drivers/net/e1000e/netdev.c +3 -0
@@ -774,6 +774,7 @@ static bool e1000_clean_rx_irq(struct e1if (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
@@ -1081,6 +1082,7 @@ static bool e1000_clean_rx_irq_ps(structbreak ; (*work_done)++; skb = buffer_info->skb; +rmb ();/* read descriptor and rx_buffer_info after status DD */ /* in the packet split case this is header only */ prefetch (skb->data -NET_IP_ALIGN );
@@ -1280,6 +1282,7 @@ static bool e1000_clean_jumbo_rx_irq(strif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
drivers/net/ixgb/ixgb_main.c +1 -0
@@ -1977,6 +1977,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *abreak ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb; buffer_info->skb =NULL ;
drivers/net/ixgbevf/ixgbevf_main.c +2 -0
@@ -231,6 +231,7 @@ static bool ixgbevf_clean_tx_irq(structwhile ((eop_desc->wb.status &cpu_to_le32 (IXGBE_TXD_STAT_DD )) && (count < tx_ring->work_limit)) {bool cleaned = false; +rmb ();/* read buffer_info after eop_desc */ for ( ; !cleaned; count++) {struct sk_buff *skb; tx_desc =IXGBE_TX_DESC_ADV (*tx_ring, i);
@@ -518,6 +519,7 @@ static bool ixgbevf_clean_rx_irq(structbreak ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ if (adapter->flags &IXGBE_FLAG_RX_PS_ENABLED ) { hdr_info =le16_to_cpu (ixgbevf_get_hdr_info (rx_desc)); len = (hdr_info & IXGBE_RXDADV_HDRBUFLEN_MASK) >>
drivers/net/e100.c +1 -0
@@ -1928,6 +1928,7 @@ static int e100_rx_indicate(struct nic *netif_printk (nic, rx_status,KERN_DEBUG , nic->netdev,"status=0x%04X\n" , rfd_status); +rmb ();/* read size after status bit */ /* If data isn't ready, nothing to indicate */ if (unlikely (!(rfd_status & cb_complete))) {
Quoted 108 lines · Sonny Rao
Add missing memory barriers to
clean_rx_irqfunctions in Intel DriversThis patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf I've also added a missing barrier to the
clean_tx_irqpath because I missed it in my last patch.Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
v2: I missed the e100 the first time
Signed-off-by Milton Miller <[email protected]>Signed-off-by Sonny Rao <[email protected]>cc stable <[email protected]>Index:
linux-2.6.35-rc5/drivers/net/e1000/e1000_main.c
===================================================================drivers/net/e1000/e1000_main.c 2010-07-27 16:22:21.000000000 -0500 renamed from drivers/net/e1000/e1000_main.c 2010-07-27 16:15:18.000000000 -0500 +2
-0@@ -3638,6 +3638,7 @@ static bool e1000_clean_jumbo_rx_irq(str if (*work_done >= work_to_do) break; (*work_done)++; + rmb(); /* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;@@ -3844,6 +3845,7 @@ static bool e1000_clean_rx_irq(struct e1 if (*work_done >= work_to_do) break; (*work_done)++; + rmb(); /* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;Index:
linux-2.6.35-rc5/drivers/net/e1000e/netdev.c
===================================================================drivers/net/e1000e/netdev.c 2010-07-27 16:25:23.000000000 -0500 renamed from drivers/net/e1000e/netdev.c 2010-07-27 16:22:38.000000000 -0500 +3
-0@@ -774,6 +774,7 @@ static bool e1000_clean_rx_irq(struct e1 if (*work_done >= work_to_do) break; (*work_done)++; + rmb(); /* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;@@ -1081,6 +1082,7 @@ static bool e1000_clean_rx_irq_ps(struct break; (*work_done)++; skb = buffer_info->skb; + rmb(); /* read descriptor and rx_buffer_info after status DD */ /* in the packet split case this is header only */ prefetch(skb->data - NET_IP_ALIGN);@@ -1280,6 +1282,7 @@ static bool e1000_clean_jumbo_rx_irq(str if (*work_done >= work_to_do) break; (*work_done)++; + rmb(); /* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;Index:
linux-2.6.35-rc5/drivers/net/ixgb/ixgb_main.c
===================================================================drivers/net/ixgb/ixgb_main.c 2010-07-27 16:41:57.000000000 -0500 renamed from drivers/net/ixgb/ixgb_main.c 2010-07-27 16:27:23.000000000 -0500 +1
-0@@ -1977,6 +1977,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *a break; (*work_done)++; + rmb(); /* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb; buffer_info->skb = NULL;Index:
linux-2.6.35-rc5/drivers/net/ixgbevf/ixgbevf_main.c
===================================================================drivers/net/ixgbevf/ixgbevf_main.c 2010-07-27 16:40:19.000000000 -0500 renamed from drivers/net/ixgbevf/ixgbevf_main.c 2010-07-27 16:30:51.000000000 -0500 +0
-0@@ -231,6 +231,7 @@ static bool ixgbevf_clean_tx_irq(structwhile ((
eop_desc->wb.status&cpu_to_le32(IXGBE_TXD_STAT_DD)) && (count <tx_ring->work_limit)) {
bool cleaned = false;
+rmb(); /* readbuffer_infoaftereop_desc*/ for ( ; !cleaned; count++) {
structsk_buff*skb;
tx_desc =IXGBE_TX_DESC_ADV(*tx_ring, i);drivers/net/ixgbevf/ixgbevf_main.c 2010-07-27 16:40:19.000000000 -0500 renamed from drivers/net/ixgbevf/ixgbevf_main.c 2010-07-27 16:30:51.000000000 -0500 +1
-0@@ -518,6 +519,7 @@ static bool ixgbevf_clean_rx_irq(struct break; (*work_done)++; + rmb(); /* read descriptor and rx_buffer_info after status DD */ if (adapter->flags & IXGBE_FLAG_RX_PS_ENABLED) { hdr_info = le16_to_cpu(ixgbevf_get_hdr_info(rx_desc));len = (
hdr_info&IXGBE_RXDADV_HDRBUFLEN_MASK) >>
Index:linux-2.6.35-rc5/drivers/net/e100.c
===================================================================
drivers/net/e100.c 2010-07-27 17:41:38.000000000 -0500 static int e100_rx_indicate(struct nic *
Quoted 8 lines
drivers/net/e100.c 2010-07-27 17:41:38.000000000 -0500 renamed from drivers/net/e100.c 2010-07-27 17:36:44.000000000 -0500 +1
-1@@ -1928,6 +1928,7 @@ static int e100_rx_indicate(struct nic * netif_printk(nic, rx_status, KERN_DEBUG, nic->netdev, "status=0x%04X\n", rfd_status); + rmb(); /* read size after status bit */ /* If data isn't ready, nothing to indicate */ if (unlikely(!(rfd_status & cb_complete))) { --
You also seem to be missing igb.
Signature · 2 lines
Cheers, Jeff
Quoted 3 lines · Jeff Kirsher
Quoted 1 line
--
You also seem to be missing igb.
You're right, at first I looked at igb and didn't see the analgous bit of code but I apparently didn't look hard enough.. stand by for version 3
Signature · 1 line
Sonny Rao, LTC OzLabs, BML team
PATCH v3 1/1 Add missing memory barriers to clean_rx_irq functions in Intel Drivers
In reply to Jeff KirsherQuoted 2 lines · Jeff Kirsher
You also seem to be missing igb.
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf, igb, and igbvf I've also added a missing barrier to the clean_tx_irq path because I missed it in my last patch.
Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
v2: I missed the e100 the first time
v3: I missed igb and igbvf, third time's the charm?
drivers/net/e1000/e1000_main.c +2 -0
@@ -3638,6 +3638,7 @@ static bool e1000_clean_jumbo_rx_irq(strif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
@@ -3844,6 +3845,7 @@ static bool e1000_clean_rx_irq(struct e1if (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
drivers/net/e1000e/netdev.c +3 -0
@@ -774,6 +774,7 @@ static bool e1000_clean_rx_irq(struct e1if (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
@@ -1081,6 +1082,7 @@ static bool e1000_clean_rx_irq_ps(structbreak ; (*work_done)++; skb = buffer_info->skb; +rmb ();/* read descriptor and rx_buffer_info after status DD */ /* in the packet split case this is header only */ prefetch (skb->data -NET_IP_ALIGN );
@@ -1280,6 +1282,7 @@ static bool e1000_clean_jumbo_rx_irq(strif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb;
drivers/net/ixgb/ixgb_main.c +1 -0
@@ -1977,6 +1977,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *abreak ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ status = rx_desc->status; skb = buffer_info->skb; buffer_info->skb =NULL ;
drivers/net/ixgbevf/ixgbevf_main.c +2 -0
@@ -231,6 +231,7 @@ static bool ixgbevf_clean_tx_irq(structwhile ((eop_desc->wb.status &cpu_to_le32 (IXGBE_TXD_STAT_DD )) && (count < tx_ring->work_limit)) {bool cleaned = false; +rmb ();/* read buffer_info after eop_desc */ for ( ; !cleaned; count++) {struct sk_buff *skb; tx_desc =IXGBE_TX_DESC_ADV (*tx_ring, i);
@@ -518,6 +519,7 @@ static bool ixgbevf_clean_rx_irq(structbreak ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ if (adapter->flags &IXGBE_FLAG_RX_PS_ENABLED ) { hdr_info =le16_to_cpu (ixgbevf_get_hdr_info (rx_desc)); len = (hdr_info & IXGBE_RXDADV_HDRBUFLEN_MASK) >>
drivers/net/e100.c +1 -0
@@ -1928,6 +1928,7 @@ static int e100_rx_indicate(struct nic *netif_printk (nic, rx_status,KERN_DEBUG , nic->netdev,"status=0x%04X\n" , rfd_status); +rmb ();/* read size after status bit */ /* If data isn't ready, nothing to indicate */ if (unlikely (!(rfd_status & cb_complete))) {
drivers/net/igb/igb_main.c +2 -0
@@ -5335,6 +5335,7 @@ static bool igb_clean_tx_irq(struct igb_while ((eop_desc->wb.status &cpu_to_le32 (E1000_TXD_STAT_DD )) && (count < tx_ring->count)) { +rmb ();/* read buffer_info after eop_desc status */ for (cleaned = false; !cleaned; count++) { tx_desc =E1000_TX_DESC_ADV (*tx_ring, i); buffer_info = &tx_ring->buffer_info[i];
@@ -5540,6 +5541,7 @@ static bool igb_clean_rx_irq_adv(structif (*work_done >= budget)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ skb = buffer_info->skb;prefetch (skb->data -NET_IP_ALIGN );
drivers/net/igbvf/netdev.c +2 -0
@@ -248,6 +248,7 @@ static bool igbvf_clean_rx_irq(struct igif (*work_done >= work_to_do)break ; (*work_done)++; +rmb ();/* read descriptor and rx_buffer_info after status DD */ buffer_info = &rx_ring->buffer_info[i];
@@ -780,6 +781,7 @@ static bool igbvf_clean_tx_irq(struct igwhile ((eop_desc->wb.status &cpu_to_le32 (E1000_TXD_STAT_DD )) && (count < tx_ring->count)) { +rmb ();/* read buffer_info after eop_desc status */ for (cleaned = false; !cleaned; count++) { tx_desc =IGBVF_TX_DESC_ADV (*tx_ring, i); buffer_info = &tx_ring->buffer_info[i];
Quoted 19 lines · Sonny Rao
Quoted 2 lines · Jeff Kirsher
You also seem to be missing igb.
This patch is similar to what was fixed in ixgbe in this patch:
http://marc.info/?l=e1000-devel&m=126593062701537&w=3
We should add read memory barriers to all the similar cases across the Intel ethernet driver family. In the case of ixgbevf, igb, and igbvf I've also added a missing barrier to the
clean_tx_irqpath because I missed it in my last patch.Without the barrier a processor can speculate a load ahead of the load which looks at the status bit and get stale information causing a number of different issues including invalid packet length, NULL pointers, or bad data since checksumming was assumed to be done in hardware.
Quoted 7 lines
v2: I missed the e100 the first time
v3: I missed igb and igbvf, third time's the charm?Signed-off-by Milton Miller <[email protected]>Signed-off-by Sonny Rao <[email protected]>cc stable <[email protected]>
Thanks, I have added the patch to my queue.
Signature · 2 lines
Cheers, Jeff