diff mbox series

[v2] drivers/net/b44: Change to non-atomic bit operations on pwol_mask

Message ID 20200102212706.GA29778@agluck-desk2.amr.corp.intel.com
State Accepted
Delegated to: David Miller
Headers show
Series [v2] drivers/net/b44: Change to non-atomic bit operations on pwol_mask | expand

Commit Message

Tony Luck Jan. 2, 2020, 9:27 p.m. UTC 
From: Fenghua Yu <fenghua.yu@intel.com>

Atomic operations that span cache lines are super-expensive on x86
(not just to the current processor, but also to other processes as all
memory operations are blocked until the operation completes). Upcoming
x86 processors have a switch to cause such operations to generate a #AC
trap. It is expected that some real time systems will enable this mode
in BIOS.

In preparation for this, it is necessary to fix code that may execute
atomic instructions with operands that cross cachelines because the #AC
trap will crash the kernel.

Since "pwol_mask" is local and never exposed to concurrency, there is
no need to set bits in pwol_mask using atomic operations.

Directly operate on the byte which contains the bit instead of using
__set_bit() to avoid any big endian concern due to type cast to
unsigned long in __set_bit().

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

Tony: Updated commit comment with background information motivating
this change.  No changes to code.

 drivers/net/ethernet/broadcom/b44.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

David Miller Jan. 5, 2020, 10:22 p.m. UTC | #1 
From: "Luck, Tony" <tony.luck@intel.com>
Date: Thu, 2 Jan 2020 13:27:06 -0800

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Atomic operations that span cache lines are super-expensive on x86
> (not just to the current processor, but also to other processes as all
> memory operations are blocked until the operation completes). Upcoming
> x86 processors have a switch to cause such operations to generate a #AC
> trap. It is expected that some real time systems will enable this mode
> in BIOS.
> 
> In preparation for this, it is necessary to fix code that may execute
> atomic instructions with operands that cross cachelines because the #AC
> trap will crash the kernel.
> 
> Since "pwol_mask" is local and never exposed to concurrency, there is
> no need to set bits in pwol_mask using atomic operations.
> 
> Directly operate on the byte which contains the bit instead of using
> __set_bit() to avoid any big endian concern due to type cast to
> unsigned long in __set_bit().
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>

Applied, thanks.

I wonder if this is being used in an endian safe way.  Maybe the way
the filter is written into the chip makes it work out, I don't know.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 035dbb1b2c98..ec25fd81985d 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -1516,8 +1516,10 @@  static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
 	int ethaddr_bytes = ETH_ALEN;
 
 	memset(ppattern + offset, 0xff, magicsync);
-	for (j = 0; j < magicsync; j++)
-		set_bit(len++, (unsigned long *) pmask);
+	for (j = 0; j < magicsync; j++) {
+		pmask[len >> 3] |= BIT(len & 7);
+		len++;
+	}
 
 	for (j = 0; j < B44_MAX_PATTERNS; j++) {
 		if ((B44_PATTERN_SIZE - len) >= ETH_ALEN)
@@ -1529,7 +1531,8 @@  static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
 		for (k = 0; k< ethaddr_bytes; k++) {
 			ppattern[offset + magicsync +
 				(j * ETH_ALEN) + k] = macaddr[k];
-			set_bit(len++, (unsigned long *) pmask);
+			pmask[len >> 3] |= BIT(len & 7);
+			len++;
 		}
 	}
 	return len - 1;