staging: rtl8723bs: fix comparsion to NULL - coding style

Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of testing each byte of the mac[] array for 0xff and 0x00 shortens the code and improves readability.

If np == NULL, of_get_property() returns NULL, hence the "np" check is not needed.

Instead of a fixed default mac address use a random one to reduce the likelihood of mac address collision.

Thanks to Joe Perches and Dan Carpenter.

Signed-off-by Michael Straube <[email protected]>

---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +4 -16

index e55895632921..7aa00d1391f7 100644
@@ -1177,24 +1177,13 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
                memcpy(mac, mac_addr, ETH_ALEN);
        }
 
-       if (((mac[0] == 0xff) && (mac[1] == 0xff) && (mac[2] == 0xff) &&
-            (mac[3] == 0xff) && (mac[4] == 0xff) && (mac[5] == 0xff)) ||
-           ((mac[0] == 0x00) && (mac[1] == 0x00) && (mac[2] == 0x00) &&
-            (mac[3] == 0x00) && (mac[4] == 0x00) && (mac[5] == 0x00))) {
-               if (np &&
-                   (addr = of_get_property(np, "local-mac-address", &len)) &&
+       if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
+               if ((addr = of_get_property(np, "local-mac-address", &len)) &&
                    len == ETH_ALEN) {
                        memcpy(mac_addr, addr, ETH_ALEN);
                } else {
-                       mac[0] = 0x00;
-                       mac[1] = 0xe0;
-                       mac[2] = 0x4c;
-                       mac[3] = 0x87;
-                       mac[4] = 0x00;
-                       mac[5] = 0x00;
-                       /*  use default mac addresss */
-                       memcpy(mac_addr, mac, ETH_ALEN);
-                       DBG_871X("MAC Address from efuse error, assign default one !!!\n");
+                       eth_random_addr(mac_addr);
+                       DBG_871X("MAC Address from efuse error, assign random one !!!\n");
                }
        }
 
--

2.18.0

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +3 -4

index 7aa00d1391f7..8af4a89e632f 100644
@@ -1172,15 +1172,15 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
                for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
                        mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
                }
-               memcpy(mac_addr, mac, ETH_ALEN);
+               ether_addr_copy(mac_addr, mac);
        } else{ /*      Use the mac address stored in the Efuse */
-               memcpy(mac, mac_addr, ETH_ALEN);
+               ether_addr_copy(mac, mac_addr);
        }
 
        if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
                if ((addr = of_get_property(np, "local-mac-address", &len)) &&
                    len == ETH_ALEN) {
-                       memcpy(mac_addr, addr, ETH_ALEN);
+                       ether_addr_copy(mac_addr, addr);
                } else {
                        eth_random_addr(mac_addr);
                        DBG_871X("MAC Address from efuse error, assign random one !!!\n");
--

2.18.0

Use the mac_pton() helper to convert the mac address string.

The functions key_char2num() and key_2char2num() are not used anywhere else and can be removed.

This also has the benefit of validating the input since mac_pton() returns false if the string is not valid.

Signed-off-by Michael Straube <[email protected]>

---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 30 +++----------------
 .../staging/rtl8723bs/os_dep/ioctl_linux.c    |  3 --
 2 files changed, 4 insertions(+), 29 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +4 -26

index 8af4a89e632f..8e0025e1ff14 100644
@@ -1137,25 +1137,6 @@ ParseRes rtw_ieee802_11_parse_elems(u8 *start, uint len,
 
 }
 
-static u8 key_char2num(u8 ch);
-static u8 key_char2num(u8 ch)
-{
-               if ((ch >= '0') && (ch <= '9'))
-                       return ch - '0';
-               else if ((ch >= 'a') && (ch <= 'f'))
-                       return ch - 'a' + 10;
-               else if ((ch >= 'A') && (ch <= 'F'))
-                       return ch - 'A' + 10;
-               else
-                       return 0xff;
-}
-
-u8 key_2char2num(u8 hch, u8 lch);
-u8 key_2char2num(u8 hch, u8 lch)
-{
-               return ((key_char2num(hch) << 4) | key_char2num(lch));
-}
-
 void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
 {
        u8 mac[ETH_ALEN];

@@ -1166,14 +1147,11 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
        if (!mac_addr)
                return;
 
-       if (rtw_initmac) {      /*      Users specify the mac address */
-               int jj, kk;
-
-               for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
-                       mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
-               }
+       if (rtw_initmac && mac_pton(rtw_initmac, mac)) {
+               /* Users specify the mac address */
                ether_addr_copy(mac_addr, mac);
-       } else{ /*      Use the mac address stored in the Efuse */
+       } else {
+               /* Use the mac address stored in the Efuse */
                ether_addr_copy(mac, mac_addr);
        }
 

drivers/staging/rtl8723bs/os_dep/ioctl_linux.c +0 -4

index 39502156f652..4eb51f45b387 100644
@@ -32,9 +32,6 @@
 #define WEXT_CSCAN_HOME_DWELL_SECTION  'H'
 #define WEXT_CSCAN_TYPE_SECTION                'T'
 
-
-extern u8 key_2char2num(u8 hch, u8 lch);
-
 static u32 rtw_rates[] = {1000000, 2000000, 5500000, 11000000,
        6000000, 9000000, 12000000, 18000000, 24000000, 36000000, 48000000, 54000000};
 
--

2.18.0

Quoted 7 lines · Michael Straube

Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of testing each byte of the mac[] array for 0xff and 0x00 shortens the code and improves readability.

If np == NULL, of_get_property() returns NULL, hence the "np" check is not needed.

Quoted 5 lines

Instead of a fixed default mac address use a random one to reduce the likelihood of mac address collision.

Thanks to Joe Perches and Dan Carpenter.

I guess you may use Suggested-by tag as well.

Quoted 2 lines · Michael Straube

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Suggested-by ?

Btw, ensure that the source and destination buffers are aligned to u16 as required by API.

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Michael Straube

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Suggested-by ?

I'll add it. Sorry, I was not aware of the Suggested-by tag.

Quoted 2 lines

Btw, ensure that the source and destination buffers are aligned to u16 as required by API.

To be honest I'm not sure how to do that excactly.

Use __align(2) in the array declarations? e.g.:

         u8 mac[ETH_ALEN] __align(2);

Quoted 14 lines · Michael Straube

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Michael Straube

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Suggested-by ?

I'll add it. Sorry, I was not aware of the Suggested-by tag.

Quoted 2 lines

Btw, ensure that the source and destination buffers are aligned to u16 as required by API.

Quoted 5 lines

To be honest I'm not sure how to do that excactly.

Use __align(2) in the array declarations? e.g.:

          u8 mac[ETH_ALEN] __align(2);

All initial function automatics are naturally aligned.

Quoted 1 line

Quoted 5 lines · Michael Straube

Use the mac_pton() helper to convert the mac address string.

The functions key_char2num() and key_2char2num() are not used anywhere else and can be removed.

Quoted 3 lines

This also has the benefit of validating the input since mac_pton() returns false if the string is not valid.

FWIW,

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 72 lines

Signed-off-by Michael Straube <[email protected]>

---
  .../staging/rtl8723bs/core/rtw_ieee80211.c    | 30 +++----------------
  .../staging/rtl8723bs/os_dep/ioctl_linux.c    |  3 --
  2 files changed, 4 insertions(+), 29 deletions(-)

drivers/staging/rtl8723bs/core/rtw_ieee80211.c +4 -26

index 8af4a89e632f..8e0025e1ff14 100644
@@ -1137,25 +1137,6 @@ ParseRes rtw_ieee802_11_parse_elems(u8 *start, uint len,
 
  }
 
-static u8 key_char2num(u8 ch);
-static u8 key_char2num(u8 ch)
-{
-               if ((ch >= '0') && (ch <= '9'))
-                       return ch - '0';
-               else if ((ch >= 'a') && (ch <= 'f'))
-                       return ch - 'a' + 10;
-               else if ((ch >= 'A') && (ch <= 'F'))
-                       return ch - 'A' + 10;
-               else
-                       return 0xff;
-}
-
-u8 key_2char2num(u8 hch, u8 lch);
-u8 key_2char2num(u8 hch, u8 lch)
-{
-               return ((key_char2num(hch) << 4) | key_char2num(lch));
-}
-
  void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
  {
         u8 mac[ETH_ALEN];

@@ -1166,14 +1147,11 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
         if (!mac_addr)
                 return;
 
-       if (rtw_initmac) {      /*      Users specify the mac address */
-               int jj, kk;
-
-               for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
-                       mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
-               }
+       if (rtw_initmac && mac_pton(rtw_initmac, mac)) {
+               /* Users specify the mac address */
                 ether_addr_copy(mac_addr, mac);
-       } else{ /*      Use the mac address stored in the Efuse */
+       } else {
+               /* Use the mac address stored in the Efuse */
                 ether_addr_copy(mac, mac_addr);
         }
 

drivers/staging/rtl8723bs/os_dep/ioctl_linux.c +0 -4

index 39502156f652..4eb51f45b387 100644
@@ -32,9 +32,6 @@
  #define WEXT_CSCAN_HOME_DWELL_SECTION  'H'
  #define WEXT_CSCAN_TYPE_SECTION                'T'
 
-
-extern u8 key_2char2num(u8 hch, u8 lch);
-
  static u32 rtw_rates[] = {1000000, 2000000, 5500000, 11000000,
         6000000, 9000000, 12000000, 18000000, 24000000, 36000000, 48000000, 54000000};
 
--

2.18.0

Quoted 5 lines · Joe Perches

Quoted 4 lines · Michael Straube

Quoted 3 lines · Andy Shevchenko

Quoted 1 line · Michael Straube

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Btw, ensure that the source and destination buffers are aligned to u16 as required by API.

To be honest I'm not sure how to do that excactly. Use __align(2) in the array declarations? e.g.:

          u8 mac[ETH_ALEN] __align(2);

All initial function automatics are naturally aligned.

So there is nothing to change? Now I'm confused.

Quoted 17 lines · Joe Perches

Quoted 16 lines · Michael Straube

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Michael Straube

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Suggested-by ?

I'll add it. Sorry, I was not aware of the Suggested-by tag.

Quoted 2 lines

Btw, ensure that the source and destination buffers are aligned to u16 as required by API.

To be honest I'm not sure how to do that excactly.

Quoted 6 lines

Quoted 3 lines

Use __align(2) in the array declarations? e.g.:

          u8 mac[ETH_ALEN] __align(2);

All initial function automatics are naturally aligned.

Quoted 1 line

So there is nothing to change? Now I'm confused.

Quoted 19 lines · Joe Perches

Quoted 17 lines · Michael Straube

Quoted 16 lines · Andy Shevchenko

Quoted 7 lines

Quoted 2 lines · Michael Straube

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Suggested-by ?

I'll add it. Sorry, I was not aware of the Suggested-by tag.

Quoted 2 lines

Btw, ensure that the source and destination buffers are aligned to u16 as required by API.

To be honest I'm not sure how to do that excactly.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Use __align(2) in the array declarations? e.g.:

           u8 mac[ETH_ALEN] __align(2);

All initial function automatics are naturally aligned.

Quoted 1 line

So there is nothing to change? Now I'm confused.

Do not add the __align(2), as Joe says, it's not required. You just need to C alignment rules (it's expected/required for this sort of patch).

Like if you have a struct:

struct foo {
	char a;
	int b;
};

There is going to be a 3 byte gap between a and b because ints are normally __align(4). The exception is when the struct is __packed. So sizeof(struct foo) in this case is going to be 8. kmalloc() returns pointers which are 8 at least byte aligned normally. See ARCH_KMALLOC_MINALIGN. There is one arch where it's 4 byte aligned?

So when you would get things which aren't __align(2) is when you have:

struct bar {
	char a[3];
	u8 mac[ETH_ALEN];
};

Here the struct member before the mac[] is an odd number of char. Or when the struct is packed.

regards,
dan carpenter

Should I add a thanks line to the commit message:

Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.

Or would that be considered as too much?

Quoted 5 lines · Michael Straube

Should I add a thanks line to the commit message:

Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.

Or would that be considered as too much?

You can write whatever the heck you want... :P No one cares.

When it comes to credit, I do appreciate Reported-by tags because LWN and employers do count those sometimes.

Quoted 8 lines · Dan Carpenter

Quoted 5 lines · Michael Straube

Should I add a thanks line to the commit message:

Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.

Or would that be considered as too much?

You can write whatever the heck you want... :P No one cares.

Quoted 2 lines

When it comes to credit, I do appreciate Reported-by tags because LWN and employers do count those sometimes.

In some cases Suggested-by fits better.

Quoted 9 lines · Andy Shevchenko

Quoted 8 lines · Dan Carpenter

Quoted 5 lines · Michael Straube

Should I add a thanks line to the commit message:

Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.

Or would that be considered as too much?

You can write whatever the heck you want... :P No one cares.

I hope that's true.

Quoted 2 lines

Quoted 2 lines

When it comes to credit, I do appreciate Reported-by tags because LWN and employers do count those sometimes.

Which is likely as useful as judging engineer
productivity by LOC counts.