Skip to contentSkip to conversation

rt2x00: optimize mac/bssid writing

PATCH In review Ivo van Doorn 2 participants 7 messages Feb 28, 2007 Networking drivers (wireless)

lore
Share & tools
lore

Ivo van Doorn

author
#0 lore raw

PATCH 8/28 rt2x00: optimize mac/bssid writing

Handling the mac and bssid configuration can be done much easier by writing the passed data directly into the register instead of moving it to a local variable first.

Signed-off-by Ivo van Doorn <[email protected]>

---

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +2 -8
index 27e151d..b6bf9f3 100644
@@ -319,14 +319,11 @@ static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2400pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);
 }
 
 static void rt2400pci_config_promisc(struct rt2x00_dev *rt2x00dev, int promisc)

@@ -674,14 +671,11 @@ static void rt2400pci_config_phymode(struct rt2x00_dev *rt2x00dev,
 static void rt2400pci_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	void *addr)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The MAC address is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 */
-	memcpy(&reg, addr, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR3, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR3, (u32*)addr, ETH_ALEN);
 }
 
 /*
drivers/net/wireless/mac80211/rt2x00/rt2500pci.c +2 -8
index 6234d20..69ada0e 100644
@@ -319,14 +319,11 @@ static inline void rt2500pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2500pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);
 }
 
 static void rt2500pci_config_promisc(struct rt2x00_dev *rt2x00dev, int promisc)

@@ -749,14 +746,11 @@ static void rt2500pci_config_phymode(struct rt2x00_dev *rt2x00dev,
 static void rt2500pci_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	void *addr)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The MAC address is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 */
-	memcpy(&reg, addr, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR3, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR3, (u32*)addr, ETH_ALEN);
 }
 
 /*
drivers/net/wireless/mac80211/rt2x00/rt2500usb.c +2 -8
index c8c0f5a..86c06ac 100644
@@ -373,14 +373,11 @@ static inline void rt2500usb_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2500usb_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u16 reg[3] = { 0, 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR5, (u16*)bssid, ETH_ALEN);
 }
 
 static void rt2500usb_config_promisc(struct rt2x00_dev *rt2x00dev, int promisc)

@@ -781,14 +778,11 @@ static void rt2500usb_config_phymode(struct rt2x00_dev *rt2x00dev,
 static void rt2500usb_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	void *addr)
 {
-	u16 reg[3] = { 0, 0, 0 };
-
 	/*
 	 * The MAC address is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 */
-	memcpy(&reg, addr, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR2, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR2, (u16*)addr, ETH_ALEN);
 }
 
 /*
drivers/net/wireless/mac80211/rt2x00/rt61pci.c +12 -8
index 4224a57..2f7d18e 100644
@@ -351,16 +351,18 @@ static inline void rt61pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt61pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
+	u32 reg;
 
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 * We only need to set the BSS ID MASK at the correct offset.
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_set_field32(&reg[1], MAC_CSR5_BSS_ID_MASK, 3);
-	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR4, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR4, (u32*)bssid, ETH_ALEN);
+
+	rt2x00_register_read(rt2x00dev, MAC_CSR5, &reg);
+	rt2x00_set_field32(&reg, MAC_CSR5_BSS_ID_MASK, 3);
+	rt2x00_register_write(rt2x00dev, MAC_CSR5, reg);
 }
 
 static void rt61pci_config_promisc(struct rt2x00_dev *rt2x00dev, int promisc)

@@ -970,7 +972,7 @@ static void rt61pci_config_phymode(struct rt2x00_dev *rt2x00dev,
 static void rt61pci_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	void *addr)
 {
-	u32 reg[2] = { 0, 0 };
+	u32 reg;
 
 	/*
 	 * The MAC address is passed to us as an array of bytes,

@@ -978,9 +980,11 @@ static void rt61pci_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	 * We only need to set the MAC_CSR3_UNICAST_TO_ME_MASK
 	 * at the correct offset.
 	 */
-	memcpy(&reg, addr, ETH_ALEN);
-	rt2x00_set_field32(&reg[1], MAC_CSR3_UNICAST_TO_ME_MASK, 0xff);
-	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR2, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR2, (u32*)addr, ETH_ALEN);
+
+	rt2x00_register_read(rt2x00dev, MAC_CSR3, &reg);
+	rt2x00_set_field32(&reg, MAC_CSR3_UNICAST_TO_ME_MASK, 0xff);
+	rt2x00_register_write(rt2x00dev, MAC_CSR3, reg);
 }
 
 /*
drivers/net/wireless/mac80211/rt2x00/rt73usb.c +12 -8
index ef16d82..f0fff49 100644
@@ -358,16 +358,18 @@ static inline void rt73usb_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt73usb_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
+	u32 reg;
 
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.
 	 * We only need to set the BSS ID MASK at the correct offset.
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_set_field32(&reg[1], MAC_CSR5_BSS_ID_MASK, 3);
-	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR4, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR4, (u32*)bssid, ETH_ALEN);
+
+	rt2x00_register_read(rt2x00dev, MAC_CSR5, &reg);
+	rt2x00_set_field32(&reg, MAC_CSR5_BSS_ID_MASK, 3);
+	rt2x00_register_write(rt2x00dev, MAC_CSR5, reg);
 }
 
 static void rt73usb_config_promisc(struct rt2x00_dev *rt2x00dev, int promisc)

@@ -868,7 +870,7 @@ static void rt73usb_config_phymode(struct rt2x00_dev *rt2x00dev,
 static void rt73usb_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	void *addr)
 {
-	u32 reg[2] = { 0, 0 };
+	u32 reg;
 
 	/*
 	 * The MAC address is passed to us as an array of bytes,

@@ -876,9 +878,11 @@ static void rt73usb_config_mac_address(struct rt2x00_dev *rt2x00dev,
 	 * We only need to set the MAC_CSR3_UNICAST_TO_ME_MASK
 	 * at the correct offset.
 	 */
-	memcpy(&reg, addr, ETH_ALEN);
-	rt2x00_set_field32(&reg[1], MAC_CSR3_UNICAST_TO_ME_MASK, 0xff);
-	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR2, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, MAC_CSR2, (u32*)addr, ETH_ALEN);
+
+	rt2x00_register_read(rt2x00dev, MAC_CSR3, &reg);
+	rt2x00_set_field32(&reg, MAC_CSR3_UNICAST_TO_ME_MASK, 0xff);
+	rt2x00_register_write(rt2x00dev, MAC_CSR3, reg);
 }
 
 /*
#1 lore raw
Quoted 13 lines · Ivo van Doorn

Handling the mac and bssid configuration can be done much easier by writing the passed data directly into the register instead of moving it to a local variable first.

Signed-off-by Ivo van Doorn <[email protected]>

---

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -0
index 27e151d..b6bf9f3 100644

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 324

Quoted 8 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -2
@@ -319,14 +319,11 @@ static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2400pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.

           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 326

Quoted 4 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +1 -2
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);

                                                    ^^^^^^

This doesn't break on BE machines?

[Snipped the rest of the patch. Same comment applies there, too]

Signature · 1 line
Greetings Michael.
author
#2 lore raw
Quoted 23 lines · Michael Buesch
Quoted 13 lines · Ivo van Doorn

Handling the mac and bssid configuration can be done much easier by writing the passed data directly into the register instead of moving it to a local variable first.

Signed-off-by Ivo van Doorn <[email protected]>

---

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -0
index 27e151d..b6bf9f3 100644

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 324

Quoted 8 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -2
@@ -319,14 +319,11 @@ static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2400pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.

           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 7 lines

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 326

Quoted 4 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +1 -2
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);

                                                    ^^^^^^

This doesn't break on BE machines?

No, the multiwrite (just like multiread) does not perform byteordering, so the device will receive each byte in the correct order.

Ivo

#3 lore raw
Quoted 30 lines · Ivo van Doorn
Quoted 29 lines · Michael Buesch
Quoted 13 lines · Ivo van Doorn

Handling the mac and bssid configuration can be done much easier by writing the passed data directly into the register instead of moving it to a local variable first.

Signed-off-by Ivo van Doorn <[email protected]>

---

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -0
index 27e151d..b6bf9f3 100644

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 324

Quoted 8 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -2
@@ -319,14 +319,11 @@ static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2400pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.

           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 326

Quoted 4 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +1 -2
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);

                                                    ^^^^^^
Quoted 4 lines
Quoted 1 line

This doesn't break on BE machines?

No, the multiwrite (just like multiread) does not perform byteordering, so the device will receive each byte in the correct order.

No wait. It's not about performing byteordering somewhere. It's that casting a bytearray (which is little endian) into an u32 array (which is CPU endian) is almost always wrong.

Do you writel this array in a loop inside of register_multiwrite? If yes, it's broken. writel expects values in CPU endianess. This array will always be little endian.

Signature · 1 line
Greetings Michael.
author
#4 lore raw
Quoted 36 lines · Michael Buesch
Quoted 30 lines · Ivo van Doorn
Quoted 29 lines · Michael Buesch
Quoted 13 lines · Ivo van Doorn

Handling the mac and bssid configuration can be done much easier by writing the passed data directly into the register instead of moving it to a local variable first.

Signed-off-by Ivo van Doorn <[email protected]>

---

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -0
index 27e151d..b6bf9f3 100644

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 324

Quoted 8 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -2
@@ -319,14 +319,11 @@ static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2400pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.

           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 326

Quoted 4 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +1 -2
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);

                                                    ^^^^^^
Quoted 4 lines
Quoted 1 line

This doesn't break on BE machines?

No, the multiwrite (just like multiread) does not perform byteordering, so the device will receive each byte in the correct order.

Quoted 7 lines

No wait. It's not about performing byteordering somewhere. It's that casting a bytearray (which is little endian) into an u32 array (which is CPU endian) is almost always wrong.

Do you writel this array in a loop inside of register_multiwrite? If yes, it's broken. writel expects values in CPU endianess. This array will always be little endian.

register_multiread uses memcpy_fromio(), and
register_multiwrite uses memcpy_toio().
So that should be safe to use right?
Otherwise would making register_multiwrite accept a void* pointer be better?

Ivo

#5 lore raw
Quoted 41 lines · Ivo van Doorn
Quoted 40 lines · Michael Buesch
Quoted 30 lines · Ivo van Doorn
Quoted 29 lines · Michael Buesch
Quoted 13 lines · Ivo van Doorn

Handling the mac and bssid configuration can be done much easier by writing the passed data directly into the register instead of moving it to a local variable first.

Signed-off-by Ivo van Doorn <[email protected]>

---

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -0
index 27e151d..b6bf9f3 100644

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 324

Quoted 8 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +0 -2
@@ -319,14 +319,11 @@ static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){}
  */
 static void rt2400pci_config_bssid(struct rt2x00_dev *rt2x00dev, u8 *bssid)
 {
-	u32 reg[2] = { 0, 0 };
-
 	/*
 	 * The BSSID is passed to us as an array of bytes,
 	 * that array is little endian, so no need for byte ordering.

           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

drivers/net/wireless/mac80211/rt2x00/rt2400pci.c static inline void rt2400pci_close_debugfs(struct rt2x00_dev *rt2x00dev){} · line 326

Quoted 4 lines
drivers/net/wireless/mac80211/rt2x00/rt2400pci.c +1 -2
 	 */
-	memcpy(&reg, bssid, ETH_ALEN);
-	rt2x00_register_multiwrite(rt2x00dev, CSR5, &reg[0], sizeof(reg));
+	rt2x00_register_multiwrite(rt2x00dev, CSR5, (u32*)bssid, ETH_ALEN);

                                                    ^^^^^^
Quoted 4 lines
Quoted 1 line

This doesn't break on BE machines?

No, the multiwrite (just like multiread) does not perform byteordering, so the device will receive each byte in the correct order.

No wait. It's not about performing byteordering somewhere. It's that casting a bytearray (which is little endian) into an u32 array (which is CPU endian) is almost always wrong.

Quoted 6 lines
Quoted 3 lines

Do you writel this array in a loop inside of register_multiwrite? If yes, it's broken. writel expects values in CPU endianess. This array will always be little endian.

register_multiread uses memcpy_fromio(), and
register_multiwrite uses memcpy_toio().

Ah, ok.

Quoted 1 line

So that should be safe to use right?

Yes.

Quoted 1 line

Otherwise would making register_multiwrite accept a void* pointer be better?

Yeah it would. The argument type is wrong. If the function uses memcpy_foo it expects an u8 array (or void array, whatever you prefer).

Signature · 1 line
Greetings Michael.
author
#6 lore raw
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

Do you writel this array in a loop inside of register_multiwrite? If yes, it's broken. writel expects values in CPU endianess. This array will always be little endian.

register_multiread uses memcpy_fromio(), and
register_multiwrite uses memcpy_toio().

Ah, ok.

Quoted 8 lines
Quoted 1 line

So that should be safe to use right?

Yes.

Quoted 1 line

Otherwise would making register_multiwrite accept a void* pointer be better?

Yeah it would. The argument type is wrong. If the function uses memcpy_foo it expects an u8 array (or void array, whatever you prefer).

Ok, thanks. I'll fix this as soon as possible.

Ivo